bug: rm refresh endpoint and require auth#6262
Draft
chloebyun-wd wants to merge 4 commits intomainfrom
Draft
Conversation
…ey, use timing-safe comparison - Pass internalRefreshKey through executeAgentFlow, executeNode, and recursive sub-flow calls so OAuth2 refresh works in Agentflows - Cache getEncryptionKey() result to avoid file I/O or AWS calls per request - Use crypto.timingSafeEqual for key comparison to prevent timing attacks Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements an internal authentication mechanism for OAuth2 token refresh requests by introducing an 'x-flowise-internal-key' header and propagating it through the execution flow. It also enhances security by omitting sensitive token data from the refresh response. Feedback suggests addressing a potential runtime error in the timing-safe comparison logic and removing the local encryption key cache to prevent consistency issues during key rotation.
…ray wrap - Use getEncryptionKey() directly instead of caching (avoids staleness on key rotation) - Compare Buffer.length instead of string.length (handles multi-byte characters) - Remove unnecessary new Uint8Array(Buffer.from(...)) double-wrapping Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
5256359 to
7d1a3f1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Original attack chain vs. current fix
POST /refresh/:idwith no authnext()→ handler runsnext()→ handler checksx-flowise-internal-keyheaderaccess_tokenprovidedKeyisundefined→ returns 401 immediatelytokenInfo: { ...tokenData }leaksaccess_tokenhas_new_refresh_tokenandexpires_atThe Docker validation test (
POST /refresh/fake-uuidreturning{"message":"Credential not found"}instead of401) would now return{"success":false,"message":"Unauthorized"}— a 401 before the credential lookup even happens.All five reported impacts mitigated
x-flowise-internal-key→ attacker gets 401 before any credential is touchedaccess_token/refresh_tokenare stripped from the response bodyDefense in depth layers
crypto.timingSafeEqual— primary gate; prevents timing side-channel attacks on key comparisongetEncryptionKey()— supports env var, AWS Secrets Manager, and key file (no single-point-of-failure)options.internalRefreshKey— server resolves the key once and passes it through the flow execution pipeline, so components don't depend onprocess.envdirectlyArchitectural trade-off
The endpoint remains in
WHITELIST_URLSso the global auth middleware doesn't block internal server-to-server calls (which carry no JWT/API key). This is acceptable because the handler's own auth check runs first, using the server's encryption key — which an external attacker cannot know.