feat: add signed URL generation with expiration support#191
Open
IAMSDR wants to merge 2 commits intoEverythingSuckz:mainfrom
Open
feat: add signed URL generation with expiration support#191IAMSDR wants to merge 2 commits intoEverythingSuckz:mainfrom
IAMSDR wants to merge 2 commits intoEverythingSuckz:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds URL signing for stream links: new generate endpoint to create signed URLs with optional expiry, utilities to sign/verify URLs, stream route updated to accept exp+sig verification (skipping legacy hash check when used), and a new HMAC_SECRET config value with sample env entry. Changes
Sequence DiagramsequenceDiagram
participant Client
participant GenerateRoute as Generate Route
participant Config
participant Utils
participant StreamRoute as Stream Route
Client->>GenerateRoute: GET /generate/:messageID?token=...&exp=...
GenerateRoute->>Config: validate token / bot token
Config-->>GenerateRoute: auth OK
GenerateRoute->>Utils: SignURL(messageID, expiresAt)
Utils-->>GenerateRoute: signature
GenerateRoute-->>Client: 200 JSON {url, expires_at, expires_in} (or 302 redirect)
Client->>StreamRoute: GET /stream?messageID=X&sig=Y&exp=Z
StreamRoute->>Utils: VerifyURL(sig, messageID, exp)
Utils-->>StreamRoute: valid / invalid
StreamRoute-->>Client: 200 stream content / 403 forbidden
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/routes/generate.go (1)
23-28: Handle JSON encoding failures inwriteJSON.
enc.Encode(v)error is currently ignored; return a fallback 500 if encoding fails.♻️ Proposed hardening
func writeJSON(ctx *gin.Context, status int, v any) { buf := &bytes.Buffer{} enc := json.NewEncoder(buf) enc.SetEscapeHTML(false) - enc.Encode(v) + if err := enc.Encode(v); err != nil { + ctx.Data(http.StatusInternalServerError, "application/json; charset=utf-8", []byte(`{"ok":false,"error":"internal error"}`)) + return + } ctx.Data(status, "application/json; charset=utf-8", buf.Bytes()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/routes/generate.go` around lines 23 - 28, The writeJSON helper ignores encoding errors — modify writeJSON to check the error returned by enc.Encode(v) and handle failures by returning a 500 response instead of sending potentially empty/partial bytes; specifically, in function writeJSON, after calling enc.Encode(v) capture the error, and if non-nil call ctx.Data or ctx.JSON with HTTP 500 (e.g., "application/json; charset=utf-8") and a minimal JSON error body or message, ensuring you still call enc.SetEscapeHTML(false) before encoding and avoid writing the original buf.Bytes() when encoding failed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/routes/generate.go`:
- Around line 57-68: The parsed duration from time.ParseDuration can be
non-positive (e.g., "-1h" or "0s") which allows immediately expired links; after
parsing expParam in the block that sets expiryDuration, add a validation that
rejects d <= 0 unless the raw expParam is exactly "0" (explicitly allow the
special-case of exp=0). Concretely, in the branch that handles expParam (where
you call time.ParseDuration and assign expiryDuration), check if d <= 0 &&
expParam != "0" and return a 400 with an "invalid exp duration" error; otherwise
allow d (or treat "0" as zero-duration) and continue to compute expiresAt and
expiryLabel as before.
- Around line 39-44: The handler currently accepts a token from URL query via
ctx.Query("token") which risks credential leakage; update the auth logic in this
handler to only read the token from the header (ctx.GetHeader("X-Bot-Token"))
and remove the ctx.Query("token") check and any fallback behavior so that
comparison against config.ValueOf.BotToken is performed only against the header
value; ensure the unauthorized response (writeJSON(..., http.StatusUnauthorized,
...)) remains intact when the header is missing or does not match.
In `@internal/utils/hashing.go`:
- Around line 39-40: Update the expiry boundary check to reject links at the
exact expiry second by changing the condition that currently uses
time.Now().Unix() > expiry to use >= instead; in the block where expiry is
compared (the if that returns "link has expired", false when expired), replace
the strict greater-than comparison with greater-than-or-equal so that when now
>= expiry the function returns the expired result.
- Around line 27-32: SignURL currently truncates the HMAC-SHA256 output using
config.ValueOf.HashLength (often 6), producing weak auth tokens; change SignURL
to enforce a security-bounded minimum truncation length (e.g., define a constant
MinAuthHMACHexLen = 32 or use a dedicated config.ValueOf.AuthSignatureLength)
and use max(config.ValueOf.HashLength, MinAuthHMACHexLen) or the dedicated
AuthSignatureLength when slicing the hex string. Update the SignURL function to
compute the full HMAC-SHA256 hex, then truncate using the enforced length (never
below the minimum) and keep the display-only HashLength separate for public
hashes.
---
Nitpick comments:
In `@internal/routes/generate.go`:
- Around line 23-28: The writeJSON helper ignores encoding errors — modify
writeJSON to check the error returned by enc.Encode(v) and handle failures by
returning a 500 response instead of sending potentially empty/partial bytes;
specifically, in function writeJSON, after calling enc.Encode(v) capture the
error, and if non-nil call ctx.Data or ctx.JSON with HTTP 500 (e.g.,
"application/json; charset=utf-8") and a minimal JSON error body or message,
ensuring you still call enc.SetEscapeHTML(false) before encoding and avoid
writing the original buf.Bytes() when encoding failed.
internal/routes/generate.go
Outdated
Comment on lines
+39
to
+44
| token := ctx.Query("token") | ||
| if token == "" { | ||
| token = ctx.GetHeader("X-Bot-Token") | ||
| } | ||
| if token == "" || token != config.ValueOf.BotToken { | ||
| writeJSON(ctx, http.StatusUnauthorized, gin.H{"ok": false, "error": "unauthorized"}) |
There was a problem hiding this comment.
Do not accept bot token via query parameter.
Allowing token in URL leaks credentials through logs, browser history, and referrers. Require header-only auth for this endpoint.
🛡️ Proposed fix
- token := ctx.Query("token")
- if token == "" {
- token = ctx.GetHeader("X-Bot-Token")
- }
+ token := ctx.GetHeader("X-Bot-Token")
if token == "" || token != config.ValueOf.BotToken {
writeJSON(ctx, http.StatusUnauthorized, gin.H{"ok": false, "error": "unauthorized"})
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| token := ctx.Query("token") | |
| if token == "" { | |
| token = ctx.GetHeader("X-Bot-Token") | |
| } | |
| if token == "" || token != config.ValueOf.BotToken { | |
| writeJSON(ctx, http.StatusUnauthorized, gin.H{"ok": false, "error": "unauthorized"}) | |
| token := ctx.GetHeader("X-Bot-Token") | |
| if token == "" || token != config.ValueOf.BotToken { | |
| writeJSON(ctx, http.StatusUnauthorized, gin.H{"ok": false, "error": "unauthorized"}) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/routes/generate.go` around lines 39 - 44, The handler currently
accepts a token from URL query via ctx.Query("token") which risks credential
leakage; update the auth logic in this handler to only read the token from the
header (ctx.GetHeader("X-Bot-Token")) and remove the ctx.Query("token") check
and any fallback behavior so that comparison against config.ValueOf.BotToken is
performed only against the header value; ensure the unauthorized response
(writeJSON(..., http.StatusUnauthorized, ...)) remains intact when the header is
missing or does not match.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
config/config.go (2)
114-117: Handle flag read errors and normalize secret input.Line 114 currently ignores
GetStringerrors and accepts whitespace-only secrets. For a security-sensitive value, fail-safe parsing is better.Proposed hardening patch
- hmacSecret, _ := cmd.Flags().GetString("hmac-secret") - if hmacSecret != "" { - os.Setenv("HMAC_SECRET", hmacSecret) - } + hmacSecret, err := cmd.Flags().GetString("hmac-secret") + if err != nil { + log.Warn("Failed to read --hmac-secret flag", zap.Error(err)) + } else if strings.TrimSpace(hmacSecret) != "" { + os.Setenv("HMAC_SECRET", strings.TrimSpace(hmacSecret)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/config.go` around lines 114 - 117, Check and handle the error returned by cmd.Flags().GetString when reading "hmac-secret" and normalize the secret by trimming whitespace before accepting it; specifically, replace the current blind call that assigns to hmacSecret with error handling for cmd.Flags().GetString("hmac-secret"), then apply strings.TrimSpace(hmacSecret) and only call os.Setenv("HMAC_SECRET", hmacSecret) if the trimmed secret is non-empty—otherwise return or exit with an explicit error so a whitespace-only or failed-flag read cannot set an insecure value.
180-182: Add an explicit warning when using BOT_TOKEN as signing key.Line 180 fallback is functional, but silently coupling signing and bot auth secrets can complicate key rotation and incident response.
Proposed observability patch
if c.HMACSecret == "" { c.HMACSecret = c.BotToken + log.Sugar().Warn("HMAC_SECRET is unset; falling back to BOT_TOKEN. Set HMAC_SECRET explicitly to decouple URL-signing key rotation.") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/config.go` around lines 180 - 182, If c.HMACSecret falls back to c.BotToken (the current conditional setting using c.HMACSecret == ""), emit an explicit warning informing operators that BOT_TOKEN is being used as the signing key and advising against it for key rotation/incident response; update the fallback branch in the same block to log a warned message (e.g., using the project's logger or fmt/log package) that includes which fields are used (referencing c.HMACSecret and c.BotToken) and recommends configuring a distinct HMAC secret, while still keeping the existing fallback assignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@config/config.go`:
- Around line 114-117: Check and handle the error returned by
cmd.Flags().GetString when reading "hmac-secret" and normalize the secret by
trimming whitespace before accepting it; specifically, replace the current blind
call that assigns to hmacSecret with error handling for
cmd.Flags().GetString("hmac-secret"), then apply strings.TrimSpace(hmacSecret)
and only call os.Setenv("HMAC_SECRET", hmacSecret) if the trimmed secret is
non-empty—otherwise return or exit with an explicit error so a whitespace-only
or failed-flag read cannot set an insecure value.
- Around line 180-182: If c.HMACSecret falls back to c.BotToken (the current
conditional setting using c.HMACSecret == ""), emit an explicit warning
informing operators that BOT_TOKEN is being used as the signing key and advising
against it for key rotation/incident response; update the fallback branch in the
same block to log a warned message (e.g., using the project's logger or fmt/log
package) that includes which fields are used (referencing c.HMACSecret and
c.BotToken) and recommends configuring a distinct HMAC secret, while still
keeping the existing fallback assignment.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config/config.gofsb.sample.envinternal/routes/generate.gointernal/utils/hashing.go
✅ Files skipped from review due to trivial changes (1)
- fsb.sample.env
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/routes/generate.go
- internal/utils/hashing.go
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
1 participant
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.
🚀 Summary of Changes
/generate/:messageIDrouteParameters
messageID→ Telegram message ID of the filesecret→ HMAC secret (required, via query param or X-HMAC-Secret headerexp→ Expiration duration (e.g.,1h,30m,24h)redirect→ Optional (trueto directly redirect)exp(expiry timestamp) andsig(HMAC signature) parameters ( optional)🎯 Motivation
I am using this project in my own personal setup, and I noticed that generated links were permanent.
If a link is shared, it never expires.
This change adds a lightweight expiration system that:
The goal was to keep the solution simple and low overhead while improving link control.
I did not find documentation in the README, so documentation may need to be updated accordingly.
Summary by CodeRabbit