SCProxy.toString() rebuilds the proxy URL with user and password, and the protocol logs it
- Dominant language
- Java
- Stars
- 995
- Forks
- 292
- Avg merge
- 2d 49m
- Merged PRs (30d)
- 62
Description
## 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`:
```java
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`:
```java
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`.
```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: but was:
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.
Contributor guide
Assessment
This issue has not been assessed yet.