Skip to content

feat(proxy): allow socks proxy for the notifications#7088

Merged
CommanderStorm merged 1 commit intolouislam:masterfrom
ASLanin:allow_socks_proxy
Mar 4, 2026
Merged

feat(proxy): allow socks proxy for the notifications#7088
CommanderStorm merged 1 commit intolouislam:masterfrom
ASLanin:allow_socks_proxy

Conversation

@ASLanin
Copy link
Copy Markdown
Contributor

@ASLanin ASLanin commented Mar 4, 2026

Summary

In this pull request, the following changes are made:

Parsing NOTIFICATION_PROXY and notification_proxy env for known socks protocols and set appropriate agent.

in two words:
Simple else if added to the notification_proxy variable checking.
To be honest I tested on the socks5 only, relying on socks-proxy-agent package that way.

Please follow this checklist to avoid unnecessary back and forth (click to expand)
  • [no] ⚠️ If there are Breaking change (a fix or feature that alters existing functionality in a way that could cause issues) I have called them out
  • [yes] 🧠 I have disclosed any use of LLMs/AI in this contribution and reviewed all generated content.
    I understand that I am responsible for and able to explain every line of code I submit.
  • [no] 🔍 Any UI changes adhere to visual style of this project.
  • [yes] 🛠️ I have self-reviewed and self-tested my code to ensure it works as expected.
  • [no] 📝 I have commented my code, especially in hard-to-understand areas (e.g., using JSDoc for methods).
  • [no] 🤖 I added or updated automated tests where appropriate.
  • [no] 📄 Documentation updates are included (if applicable).
  • [no] 🧰 Dependency updates are listed and explained.
  • [no] ⚠️ CI passes and is green.

Screenshots for Visual Changes

no changes

Copilot AI review requested due to automatic review settings March 4, 2026 09:47
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 4, 2026

Hello and thanks for lending a paw to Uptime Kuma! 🐻👋
As this is your first contribution, please be sure to check out our Pull Request guidelines.
In particular: - Mark your PR as Draft while you’re still making changes - Mark it as Ready for review once it’s fully ready
If you have any design or process questions, feel free to ask them right here in this pull request - unclear documentation is a bug too.

@ASLanin ASLanin changed the title allow socks proxy for the notifications feat: allow socks proxy for the notifications Mar 4, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds SOCKS proxy support for notification HTTP requests by extending the existing NOTIFICATION_PROXY / notification_proxy env-based proxy handling in the backend notification provider.

Changes:

  • Import SocksProxyAgent for SOCKS proxy support.
  • Extend getAxiosConfigWithProxy() to detect SOCKS protocols (socks/socks4/socks5/socks5h) and apply the appropriate agent.

Comment on lines +187 to +190
} else if (["socks:", "socks4:", "socks5:", "socks5h:"].includes(proxyUrl.protocol)) {
const agent = new SocksProxyAgent(proxyEnv);
axiosConfig.httpAgent = agent;
axiosConfig.httpsAgent = agent;
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

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

The list of supported SOCKS protocols is hard-coded here, while similar protocol support is also maintained in server/proxy.js (Proxy.SUPPORTED_PROXY_PROTOCOLS). This duplication can drift over time (e.g., if supported proxy protocols are expanded/renamed in one place but not the other). Consider centralizing the supported-protocol mapping (or deriving the ...: URL protocol list from the shared constant) so notification proxy handling stays consistent with the rest of the proxy implementation.

Copilot uses AI. Check for mistakes.
Comment on lines +187 to +190
} else if (["socks:", "socks4:", "socks5:", "socks5h:"].includes(proxyUrl.protocol)) {
const agent = new SocksProxyAgent(proxyEnv);
axiosConfig.httpAgent = agent;
axiosConfig.httpsAgent = agent;
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

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

SOCKS proxy support is newly added here, but there are no backend tests asserting that getAxiosConfigWithProxy() configures the correct agent for socks/socks4/socks5/socks5h URLs (and that existing http/https behavior is unchanged). Adding a small unit test that sets process.env.NOTIFICATION_PROXY to a SOCKS URL and verifies axiosConfig.httpAgent/httpsAgent are instances of the expected agent would help prevent regressions.

Copilot generated this review using guidance from repository custom instructions.
Copy link
Copy Markdown
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

Lgtm

@CommanderStorm CommanderStorm changed the title feat: allow socks proxy for the notifications feat(proxy): allow socks proxy for the notifications Mar 4, 2026
@CommanderStorm CommanderStorm merged commit 6f74cd3 into louislam:master Mar 4, 2026
29 of 31 checks passed
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 4, 2026

@ASLanin congrats on your first contribution to Uptime Kuma! 🐻
We hope you enjoy contributing to our project and look forward to seeing more of your work in the future! If you want to see your contribution in action, please see our nightly builds here.

@CommanderStorm CommanderStorm added this to the 2.2.0 milestone Mar 6, 2026
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.

3 participants