SCProxy.toString() rebuilds the proxy URL with user and password, and the protocol logs it

[rzo1]

What happens

SCProxy.toString() reassembles the connection string it was parsed from, and puts the username and password back in when both are set. The okhttp protocol passes that string to LOG.debug once per proxied fetch. With DEBUG enabled on the protocol classes, the proxy credentials are written to the worker log on every request that uses a proxy.

Where

core/src/main/java/org/apache/stormcrawler/proxy/SCProxy.java:138-140, inside toString() at :133:

        if (this.username != null && this.password != null) {
            proxyString = this.username + ":" + this.password + "@" + proxyString;
        }

Call site, core/src/main/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocol.java:364:

                LOG.debug("fetching with proxy {} - {} ", url, prox.toString());

Why it matters

Worker logs are usually shipped to central aggregation, and the people who can read them are not the same set as the people entrusted with the proxy account. Turning on DEBUG to look at fetch behaviour is a normal thing to do, not a misconfiguration, and it writes the credential once per request. The proxy is often a paid third party service, so the credential has value on its own.

Nothing depends on toString() functionally. Proxies are used through getAddress, getPort, getUsername and getPassword; equals and hashCode are implemented on the fields, not on the rendered string. The only caller that expects the full string is SCProxyTest.testToString.

Reproduction

Save as core/src/test/java/org/apache/stormcrawler/proxy/SCProxyRedactionTest.java.

/*
 * Licensed to the Apache Software Foundation (ASF) under one or more
 * contributor license agreements.  See the NOTICE file distributed with
 * this work for additional information regarding copyright ownership.
 * The ASF licenses this file to you under the Apache License, Version 2.0
 * (the "License"); you may not use this file except in compliance with
 * the License.  You may obtain a copy of the License at
 *
 *      http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package org.apache.stormcrawler.proxy;

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

class SCProxyRedactionTest {

    @Test
    void toStringDoesNotContainCredentials() {
        SCProxy proxy = new SCProxy("http://user1:pass1@example.com:8080");
        String rendered = proxy.toString();
        Assertions.assertFalse(
                rendered.contains("pass1"),
                "SCProxy.toString() exposes the proxy password: " + rendered);
        Assertions.assertTrue(
                rendered.contains("example.com:8080"),
                "SCProxy.toString() should still identify the proxy endpoint: " + rendered);
    }
}

Run it:

mvn -pl core test -Dtest=SCProxyRedactionTest

It asserts the wanted behaviour and fails on main.

org.apache.stormcrawler.proxy.SCProxyRedactionTest.toStringDoesNotContainCredentials -- Time elapsed: 0.071 s <<< FAILURE!
org.opentest4j.AssertionFailedError: SCProxy.toString() exposes the proxy password: http://user1:pass1@example.com:8080 ==> expected: <false> but was: <true>

Tests run: 1, Failures: 1, Errors: 0, Skipped: 0

Suggested fix

Change SCProxy.toString() to render protocol://user:***@address:port, keeping the username if that is useful for support and dropping the password. Change the LOG.debug call to log getAddress() and getPort() only. SCProxyTest.testToString asserts the current round trip and has to be updated in the same commit. If some downstream code really needs the full connection string, add an explicit method for it rather than leaving it on toString(); nothing in this tree needs one today, so the round trip is the only compatibility impact.