Skip to content

chore: User REDIS URL instead of HOST and PORT#114

Merged
alexandreferris merged 1 commit into
mainfrom
chore/use_redis_url_instead_of_host_and_port
Jun 26, 2026
Merged

chore: User REDIS URL instead of HOST and PORT#114
alexandreferris merged 1 commit into
mainfrom
chore/use_redis_url_instead_of_host_and_port

Conversation

@alexandreferris

Copy link
Copy Markdown
Member

PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

Instead of trying to build the full connection url for redis, we can receive the full connection url directly.

Solutions

Use redis full connection url directly

@alexandreferris alexandreferris requested a review from a team as a code owner June 26, 2026 07:00
@alexandreferris alexandreferris self-assigned this Jun 26, 2026
Comment thread build.gradle.kts
environment("GHAPP_SERVER_PORT", "8083")
environment("GHAPP_REDIS_HOST", "redis://localhost")
environment("GHAPP_REDIS_PORT", "6379")
environment("GHAPP_REDIS_URL", "redis://localhost:6379")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semgrep identified an issue, but thinks it may be safe to ignore.
Found unencrypted Redis connection, prefer TLS encrypted rediss:// transport

Why this might be safe to ignore:

This match is in the Gradle test task and is explicitly labeled as dummy environment variables for tests, so it is not a production Redis connection. The rule intent is valid for real runtime configuration, but here it matched non-deployed test setup where changing to TLS would not meaningfully improve security.

To resolve this comment:

🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.

💬 Ignore this finding

Reply with Semgrep commands to ignore this finding.

  • /fp <comment> for false positive
  • /ar <comment> for acceptable risk
  • /other <comment> for all other reasons

Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by redis-unencrypted-transport.

You can view more details about this finding in the Semgrep AppSec Platform.

Comment thread README.md
GHAPP_SERVER_PORT=8083
GHAPP_REDIS_HOST=redis://redis
GHAPP_REDIS_PORT=6380
GHAPP_REDIS_URL=redis://username:password@host:port

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semgrep identified an issue, but thinks it may be safe to ignore.
Found unencrypted Redis connection, prefer TLS encrypted rediss:// transport

Why this might be safe to ignore:

This appears in README example environment variables, not executable application code, so it is documentation for local setup rather than an actual network connection implementation. While the example uses unencrypted Redis, changing docs may be nice, but this finding does not meaningfully improve production security by itself.

To resolve this comment:

🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.

💬 Ignore this finding

Reply with Semgrep commands to ignore this finding.

  • /fp <comment> for false positive
  • /ar <comment> for acceptable risk
  • /other <comment> for all other reasons

Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by redis-unencrypted-transport.

You can view more details about this finding in the Semgrep AppSec Platform.

Comment thread docker-compose.yml
restart: unless-stopped
ports:
- "${GHAPP_REDIS_PORT}:${GHAPP_REDIS_PORT}"
- "6379:6379"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semgrep identified an issue in your code:

Redis port 6379 is exposed on all network interfaces (0.0.0.0), allowing unauthenticated network access to your database.

More details about this

The Redis service is bound to 0.0.0.0:6379, making it accessible from any network interface including the internet. An attacker can connect directly to this port and interact with Redis without authentication (Redis has no authentication by default when not configured).

Here's an exploitation scenario:

  1. An attacker discovers your service's IP address (via DNS enumeration, nmap scanning, or other reconnaissance)
  2. The attacker runs redis-cli -h YOUR_IP_ADDRESS -p 6379 to connect to the exposed Redis instance
  3. They can immediately execute arbitrary commands like FLUSHALL (delete all data), SET (modify data), or CONFIG GET requirepass (check if authentication is enabled)
  4. If your application stores session data, user credentials, or other sensitive information in Redis, the attacker gains direct access without going through your application layer

This port mapping exposes the entire redis service to the network when it should only be accessible internally to the app container.

To resolve this comment:

✨ Commit fix suggestion

Suggested change
- "6379:6379"
- "127.0.0.1:6379:6379"
View step-by-step instructions
  1. Restrict the Redis port binding to localhost so it is not exposed on all network interfaces.
    Change ports: - "6379:6379" to ports: - "127.0.0.1:6379:6379".

  2. Keep the port mapping only if Redis must be reachable from the host machine.
    Docker services on the same Compose network can already reach Redis by service name, such as redis:6379, without publishing the port externally.

  3. Remove the ports entry entirely if only the app service needs to connect to Redis.
    In that case, keep depends_on and use the internal connection string redis://redis:6379 or the equivalent value in GHAPP_REDIS_URL.

  4. Update any host-based Redis client configuration if needed.
    If something on the host connects to Redis, point it to 127.0.0.1:6379 instead of a non-local interface.

💬 Ignore this finding

Reply with Semgrep commands to ignore this finding.

  • /fp <comment> for false positive
  • /ar <comment> for acceptable risk
  • /other <comment> for all other reasons

Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by port-all-interfaces.

You can view more details about this finding in the Semgrep AppSec Platform.

"GHAPP_REDIS_PORT",
"6379"
"GHAPP_REDIS_URL",
"redis://localhost:6379"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semgrep identified an issue in your code:

Unencrypted Redis connection using redis:// exposes all traffic (including passwords and data) to network eavesdropping attacks.

More details about this

The Redis connection URL uses the unencrypted redis:// protocol instead of the TLS-encrypted rediss:// protocol. This means all data transmitted between your application and Redis—including commands, keys, and values—is sent in plaintext over the network.

Exploit scenario:

  1. An attacker positions themselves on the network path between your application and Redis server (via ARP spoofing, BGP hijacking, or compromising network infrastructure).
  2. The attacker runs a packet sniffer like tcpdump to capture traffic: tcpdump -i eth0 'tcp port 6379'
  3. When your application connects to Redis at redis://localhost:6379, the attacker captures the connection handshake and all subsequent commands.
  4. If the Redis connection includes authentication (the commented example shows redis://[:password@]host), the attacker captures the plaintext password.
  5. The attacker can now read sensitive data stored in Redis (user sessions, cache, tokens) or execute commands to inject malicious data into your application's cache, causing data corruption or unauthorized access.

The lack of TLS encryption in ENV_VAR_REDIS_HOST leaves the entire Redis communication channel exposed to network-level attacks.

To resolve this comment:

✨ Commit fix suggestion

Suggested change
"redis://localhost:6379"
package com.wire.github.util
import java.util.UUID
/**
* Host Port to be used when setting up Ktor server.
*/
val ENV_VAR_PORT: Int = System
.getenv()
.getOrDefault(
"GHAPP_SERVER_PORT",
"8083"
).toInt()
/**
* Host URL to be used to contact via webhook.
* Must contain the protocol (`HTTP` or `HTTPS`) ???
* Note: This is not the Host URL for the Ktor server in the docker container.
*/
val ENV_VAR_HOST: String = System
.getenv()
.getOrDefault(
"GHAPP_API_HOST",
"http://0.0.0.0"
)
/**
* Redis Host URL
* In case it needs a password, must be included in this same environment variable.
* Examples:
* - "rediss://host"
* - "rediss://[:password@]host"
* Note: The configured Redis endpoint must support TLS when using `rediss://`.
*/
val ENV_VAR_REDIS_HOST: String = System
.getenv()
.getOrDefault(
"GHAPP_REDIS_HOST",
"rediss://localhost"
)
/**
* Redis Port Number
* To used when connecting and also exposed via docker-compose.
*/
val ENV_VAR_REDIS_PORT: String = System
.getenv()
.getOrDefault(
"GHAPP_REDIS_PORT",
"6379"
)
/**
* Application ID received when Onboarding the App.
*/
val ENV_VAR_APPLICATION_ID: UUID = UUID.fromString(
System
.getenv()
.getOrDefault(
"WIRE_SDK_APP_ID",
UUID.randomUUID().toString()
)
View step-by-step instructions
  1. Change the Redis URI scheme from redis:// to rediss:// anywhere this value is documented, defaulted, or constructed.
    Update the examples in the comment from redis://host and redis://[:password@]host to rediss://host and rediss://[:password@]host.

  2. Update the default Redis host value to use TLS.
    Replace the fallback value redis://localhost with rediss://localhost in the GHAPP_REDIS_HOST configuration.

  3. Keep the environment variable format consistent with the TLS scheme.
    If any deployment config, .env value, or container setting sets GHAPP_REDIS_HOST, change it to start with rediss:// instead of redis://.

  4. Confirm the Redis server accepts TLS connections on the configured endpoint.
    rediss:// enables encrypted transport, so the target Redis instance must have TLS enabled or be fronted by a TLS-terminating proxy.

  5. Alternatively, if your Redis provider only exposes a separate TLS port, point GHAPP_REDIS_HOST and the Redis port setting to that TLS-enabled endpoint instead of the plaintext one.

💬 Ignore this finding

Reply with Semgrep commands to ignore this finding.

  • /fp <comment> for false positive
  • /ar <comment> for acceptable risk
  • /other <comment> for all other reasons

Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by redis-unencrypted-transport.

You can view more details about this finding in the Semgrep AppSec Platform.

@claude

claude Bot commented Jun 26, 2026

Copy link
Copy Markdown

PR Review: Use REDIS URL instead of HOST and PORT

Thanks for this cleanup! Overall a focused, well-executed refactor. Replacing the manually-concatenated host:port string with a single connection URL is the right move — it lets Lettuce handle URL parsing (TLS via rediss://, credentials, database selection) instead of reinventing it. I verified the migration is complete and consistent: no stray references to GHAPP_REDIS_HOST / GHAPP_REDIS_PORT / hostKey / portKey remain across source, Helm, docker-compose, README, and the Gradle test config.

Code quality

  • The old code RedisClient.create("$ENV_VAR_REDIS_HOST:$ENV_VAR_REDIS_PORT") was fragile (string interpolation, easy to break the scheme). RedisClient.create(ENV_VAR_REDIS_URL) is much cleaner.
  • Default redis://localhost:6379 is sensible and matches docker-compose / the Gradle test env.

Minor nits (non-blocking)

  1. Doc/example scheme mismatch. The KDoc on ENV_VAR_REDIS_URL uses rediss://username:password@host:port (TLS) while README.md uses redis://username:password@host:port. Worth aligning, or noting in the README that rediss:// enables TLS — operators tend to copy-paste these.
  2. docker-compose redis port is now hard-coded to 6379:6379 and --port was dropped (defaults to 6379). Fine for the bundled dev instance, but it is no longer driven by env — if someone sets GHAPP_REDIS_URL to a non-6379 port expecting the bundled container to follow, it will not. Acceptable since the URL targets external Redis, but a one-line README note could save confusion.

Security

  • Credentials live in the URL sourced from a secretKeyRef in the Helm statefulset, so nothing secret is hard-coded. Usual reminder: connection URLs with embedded passwords can leak into logs if printed during startup/error handling — worth a glance if any logging touches the raw URL.

Test coverage

  • ApplicationTest mocks RedisClient/StatefulRedisConnection, so this change is not directly exercised by a test — but since URL parsing is delegated to Lettuce and no custom parsing logic remains, nothing new warrants a unit test. No action needed.

Nice, low-risk change. The nits above are optional.

@alexandreferris alexandreferris merged commit c3d8b86 into main Jun 26, 2026
12 checks passed
@alexandreferris alexandreferris deleted the chore/use_redis_url_instead_of_host_and_port branch June 26, 2026 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants