fix(oidc): don't trim trailing slash from issuer URL#79
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 14 minutes and 58 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds file-sharing: new ShareLink DB model and migrations, ShareHandler with create/revoke/access endpoints, route registration, UI for managing shares, template helpers, updated README, tests, and minor OIDC and auth/template changes. Changes
Sequence DiagramsequenceDiagram
actor Client as Anonymous Client
participant ShareHandler as ShareHandler
participant DB as Database
participant Storage as Storage Backend
participant HTTP as HTTP Response
Client->>ShareHandler: GET /s/{token}
ShareHandler->>DB: SELECT ShareLink WHERE token = ? PRELOAD File
DB-->>ShareHandler: ShareLink (or none)
alt missing/invalid/soft-deleted
ShareHandler-->>Client: 404 Not Found
else expired
ShareHandler-->>Client: 404 Not Found
else max uses set
ShareHandler->>DB: UPDATE ... SET uses = uses+1 WHERE id=? AND uses < max_uses
DB-->>ShareHandler: rowsAffected==0?
alt exhausted
ShareHandler-->>Client: 404 Not Found
else success
ShareHandler->>Storage: Open(file.Path)
Storage-->>ShareHandler: Reader, size, content-type
ShareHandler->>HTTP: Set headers (Content-Type, Content-Length, Content-Disposition)
ShareHandler->>HTTP: Stream content
HTTP-->>Client: 200 OK + file bytes
end
else unlimited uses
ShareHandler->>DB: Atomic increment uses
DB-->>ShareHandler: Success
ShareHandler->>Storage: Open(file.Path)
Storage-->>ShareHandler: Reader, size, content-type
ShareHandler->>HTTP: Set headers and stream
HTTP-->>Client: 200 OK + file bytes
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
internal/database/models/models.go (1)
66-80: Consider adding ON DELETE CASCADE constraint for the File foreign key.The
UserandFoldermodels in this file useconstraint:OnDelete:CASCADEfor their relations. TheShareLinkmodel'sFilerelation doesn't specify this, which means share links may become orphaned if a file is permanently deleted.♻️ Add cascade delete constraint
- File File `gorm:"foreignKey:FileID" json:"-"` - User User `gorm:"foreignKey:UserID" json:"-"` + File File `gorm:"foreignKey:FileID;constraint:OnDelete:CASCADE" json:"-"` + User User `gorm:"foreignKey:UserID;constraint:OnDelete:CASCADE" json:"-"`This ensures share links are automatically cleaned up when the referenced file or user is permanently deleted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/database/models/models.go` around lines 66 - 80, The ShareLink model's File relation lacks a cascade delete constraint so share links can be orphaned when a File is permanently removed; update the ShareLink struct to add a GORM constraint on the File relation (e.g., add `gorm:"foreignKey:FileID;constraint:OnDelete:CASCADE"` to the File field) so records tied to FileID are removed automatically, and apply the same `constraint:OnDelete:CASCADE` to the User field if you want share links cleaned up when users are deleted.internal/handlers/share_handler.go (1)
99-104: Consider adding a flash message on successful share link creation.Other handlers in this codebase use flash messages to provide user feedback. The redirect after creating a share link doesn't inform the user that creation succeeded.
♻️ Add success flash message
+ "github.com/agjmills/trove/internal/flash"if err := h.db.Create(&link).Error; err != nil { http.Error(w, "Internal server error", http.StatusInternalServerError) return } + flash.Success(w, "Share link created") http.Redirect(w, r, "/files/"+strconv.FormatUint(fileID, 10), http.StatusSeeOther)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handlers/share_handler.go` around lines 99 - 104, The share-link creation currently redirects after h.db.Create(&link).Error without user feedback; before calling http.Redirect(w, r, "/files/"+strconv.FormatUint(fileID, 10), http.StatusSeeOther) add the same flash-success code used in other handlers (e.g., session.AddFlash(...) or setFlash(w,r,...) and session.Save or Save(r,w)) so a success message is stored in the session for the next page render; ensure you call that flash API with a clear message like "Share link created" and persist the session before performing the redirect.internal/handlers/file_handler.go (1)
868-871: Consider filtering out expired and soft-deleted share links.The current query fetches all share links regardless of expiry or soft-deletion status. Users may see links in the UI that no longer work.
♻️ Optional: Filter active links only
// Fetch active share links for this file var shareLinks []models.ShareLink - h.db.Where("file_id = ? AND user_id = ?", file.ID, user.ID).Find(&shareLinks) + h.db.Where("file_id = ? AND user_id = ? AND deleted_at IS NULL", file.ID, user.ID). + Where("expires_at IS NULL OR expires_at > ?", time.Now()). + Find(&shareLinks)Alternatively, if showing expired links is intentional (so users can see history and revoke stale entries), consider adding visual indicators in the template to distinguish expired/exhausted links.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handlers/file_handler.go` around lines 868 - 871, The query that loads shareLinks currently returns all rows; update the query that populates shareLinks (the h.db.Where(...).Find(&shareLinks) call in file_handler.go) to exclude expired and soft-deleted links by adding conditions like "AND (expires_at IS NULL OR expires_at > ?)" with time.Now() and "AND deleted_at IS NULL" (or rely on GORM soft-delete behavior and remove Unscoped if present); pass time.Now() as the argument and ensure the models.ShareLink struct's DeletedAt field is respected so only active, non-expired links are returned.web/templates/file_view.html (1)
358-369: Add error handling for clipboard API.The
copyShareURLfunction doesn't handle the case where clipboard access fails (e.g., non-HTTPS contexts, permission denied). This could leave users without feedback.♻️ Add error handling for clipboard failure
function copyShareURL(btn, path) { const url = window.location.origin + path; navigator.clipboard.writeText(url).then(function() { const original = btn.innerHTML; btn.innerHTML = '<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"><polyline points="20 6 9 17 4 12"></polyline></svg>'; btn.classList.add('text-green-500'); setTimeout(function() { btn.innerHTML = original; btn.classList.remove('text-green-500'); }, 2000); + }).catch(function() { + // Fallback: select the input for manual copy + const input = btn.parentElement.querySelector('input[readonly]'); + if (input) { + input.select(); + input.focus(); + } }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/file_view.html` around lines 358 - 369, The copyShareURL function lacks error handling for navigator.clipboard.writeText; wrap the clipboard call in a promise catch (or use async/await try/catch) inside copyShareURL and handle failures by restoring the button state and providing user feedback (e.g., change btn.innerHTML to an error icon/text, add a 'text-red-500' class, and revert after timeout). Also consider a fallback (prompt with the URL or document.execCommand('copy')) when writeText rejects, and ensure you reference navigator.clipboard.writeText and the copyShareURL function when locating the code to update.internal/handlers/share_handler_test.go (1)
47-47: Fail fast on fixture insert errors.These
db.Create(...)calls ignore.Error; setup can silently fail and produce misleading assertions later. In test helpers, treat fixture creation failures as fatal.🧪 Suggested patch
- db.Create(user) + if err := db.Create(user).Error; err != nil { + t.Fatalf("create user: %v", err) + } ... - db.Create(f) + if err := db.Create(f).Error; err != nil { + t.Fatalf("create file: %v", err) + } ... - db.Create(other) + if err := db.Create(other).Error; err != nil { + t.Fatalf("create other user: %v", err) + } ... - db.Create(alice) + if err := db.Create(alice).Error; err != nil { + t.Fatalf("create alice: %v", err) + } ... - db.Create(user) + if err := db.Create(user).Error; err != nil { + t.Fatalf("create user: %v", err) + } ... - db.Create(link) + if err := db.Create(link).Error; err != nil { + t.Fatalf("create share link: %v", err) + }Also applies to: 65-65, 161-161, 166-166, 176-176, 207-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handlers/share_handler_test.go` at line 47, Fixture inserts using db.Create(...) in share_handler_test.go currently ignore the returned error; update each call (e.g., the db.Create(user) and other db.Create(...) occurrences) to capture the result and fail fast on error—assign the call to a variable (res := db.Create(obj)), check res.Error, and call t.Fatalf or t.Helper()+t.Fatalf with a clear message including res.Error if non-nil so test setup fails immediately instead of producing misleading assertions later.
🤖 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/handlers/share_handler_test.go`:
- Around line 31-39: The in-memory SQLite used in setupShareTest currently opens
":memory:" which can create separate DBs across connections; change the DSN
passed to sqlite.Open in the setupShareTest function to the shared-cache pattern
(e.g., "file::memory:?cache=shared") so all connections see the same in-memory
database, leaving the rest of the gorm.Open and AutoMigrate logic intact.
In `@internal/handlers/share_handler.go`:
- Around line 164-168: The code increments the share link's use count before
verifying the file exists (link.File.ID and link.File.SoftDeletedAt), which lets
unusable-file requests consume uses; move the file availability check so it runs
before any modification to the link use count and short-circuits with
http.NotFound if the file is trashed or deleted, and then only after confirming
availability perform the use-count increment/update (ideally within the same DB
transaction or with appropriate locking) in the same handler where link and
link.File are accessed.
---
Nitpick comments:
In `@internal/database/models/models.go`:
- Around line 66-80: The ShareLink model's File relation lacks a cascade delete
constraint so share links can be orphaned when a File is permanently removed;
update the ShareLink struct to add a GORM constraint on the File relation (e.g.,
add `gorm:"foreignKey:FileID;constraint:OnDelete:CASCADE"` to the File field) so
records tied to FileID are removed automatically, and apply the same
`constraint:OnDelete:CASCADE` to the User field if you want share links cleaned
up when users are deleted.
In `@internal/handlers/file_handler.go`:
- Around line 868-871: The query that loads shareLinks currently returns all
rows; update the query that populates shareLinks (the
h.db.Where(...).Find(&shareLinks) call in file_handler.go) to exclude expired
and soft-deleted links by adding conditions like "AND (expires_at IS NULL OR
expires_at > ?)" with time.Now() and "AND deleted_at IS NULL" (or rely on GORM
soft-delete behavior and remove Unscoped if present); pass time.Now() as the
argument and ensure the models.ShareLink struct's DeletedAt field is respected
so only active, non-expired links are returned.
In `@internal/handlers/share_handler_test.go`:
- Line 47: Fixture inserts using db.Create(...) in share_handler_test.go
currently ignore the returned error; update each call (e.g., the db.Create(user)
and other db.Create(...) occurrences) to capture the result and fail fast on
error—assign the call to a variable (res := db.Create(obj)), check res.Error,
and call t.Fatalf or t.Helper()+t.Fatalf with a clear message including
res.Error if non-nil so test setup fails immediately instead of producing
misleading assertions later.
In `@internal/handlers/share_handler.go`:
- Around line 99-104: The share-link creation currently redirects after
h.db.Create(&link).Error without user feedback; before calling http.Redirect(w,
r, "/files/"+strconv.FormatUint(fileID, 10), http.StatusSeeOther) add the same
flash-success code used in other handlers (e.g., session.AddFlash(...) or
setFlash(w,r,...) and session.Save or Save(r,w)) so a success message is stored
in the session for the next page render; ensure you call that flash API with a
clear message like "Share link created" and persist the session before
performing the redirect.
In `@web/templates/file_view.html`:
- Around line 358-369: The copyShareURL function lacks error handling for
navigator.clipboard.writeText; wrap the clipboard call in a promise catch (or
use async/await try/catch) inside copyShareURL and handle failures by restoring
the button state and providing user feedback (e.g., change btn.innerHTML to an
error icon/text, add a 'text-red-500' class, and revert after timeout). Also
consider a fallback (prompt with the URL or document.execCommand('copy')) when
writeText rejects, and ensure you reference navigator.clipboard.writeText and
the copyShareURL function when locating the code to update.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93bff949-cb5e-4fe7-aab9-e6b91863824e
📒 Files selected for processing (10)
README.mdinternal/database/db.gointernal/database/models/models.gointernal/handlers/file_handler.gointernal/handlers/share_handler.gointernal/handlers/share_handler_test.gointernal/oidc/provider.gointernal/routes/routes.gointernal/templateutil/templateutil.goweb/templates/file_view.html
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/handlers/oidc_handler.go (1)
55-57: Consider adding server logs on the remaining silent failure paths.A few branches now only set flash+redirect without structured logs, which may make incident debugging harder.
Suggested logging additions
if sessionState == "" || r.URL.Query().Get("state") != sessionState { + logger.Error("oidc callback state validation failed") flash.Error(w, "Login failed: invalid state parameter.") http.Redirect(w, r, "/login", http.StatusSeeOther) return } rawIDToken, ok := token.Extra("id_token").(string) if !ok { + logger.Error("oidc callback missing id_token in token response") flash.Error(w, "Authentication failed: no identity token in response.") http.Redirect(w, r, "/login", http.StatusSeeOther) return } if err := idToken.Claims(&rawClaims); err != nil { + logger.Error("oidc claims parsing failed", "error", err) flash.Error(w, "Authentication failed: could not read identity claims.") http.Redirect(w, r, "/login", http.StatusSeeOther) return } if claims.Subject == "" || claims.Email == "" { + logger.Error("oidc required claims missing", "subject_present", claims.Subject != "", "email_present", claims.Email != "") flash.Error(w, "Authentication failed: missing required claims.") http.Redirect(w, r, "/login", http.StatusSeeOther) return } if err := h.sessionManager.RenewToken(r.Context()); err != nil { + logger.Error("oidc session renewal failed", "error", err) flash.Error(w, "Failed to create session. Please try again.") http.Redirect(w, r, "/login", http.StatusSeeOther) return }Also applies to: 73-75, 88-90, 95-97, 111-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handlers/oidc_handler.go` around lines 55 - 57, Several branches in internal/handlers/oidc_handler.go currently only call flash.Error + http.Redirect (e.g., the blocks containing flash.Error and http.Redirect at the lines shown and the similar branches at 73-75, 88-90, 95-97, 111-113) and need structured server logging; for each of those failure paths add a log entry (using the project's logger or log.Printf) immediately before the flash/redirect that records the handler name (OIDC callback/handler), the failure reason, relevant contextual info (request URL, r.RemoteAddr, user/session id if present) and any error value available (e.g., token exchange error or userinfo error), so the code paths that call flash.Error(...) then http.Redirect(...) also call logger.Errorf/log.Printf(...) with the error details.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/handlers/oidc_handler.go`:
- Around line 55-57: Several branches in internal/handlers/oidc_handler.go
currently only call flash.Error + http.Redirect (e.g., the blocks containing
flash.Error and http.Redirect at the lines shown and the similar branches at
73-75, 88-90, 95-97, 111-113) and need structured server logging; for each of
those failure paths add a log entry (using the project's logger or log.Printf)
immediately before the flash/redirect that records the handler name (OIDC
callback/handler), the failure reason, relevant contextual info (request URL,
r.RemoteAddr, user/session id if present) and any error value available (e.g.,
token exchange error or userinfo error), so the code paths that call
flash.Error(...) then http.Redirect(...) also call logger.Errorf/log.Printf(...)
with the error details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c00a0bb-d5c4-4714-9335-f1089b2f2a5b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.modinternal/handlers/auth.gointernal/handlers/oidc_handler.go
✅ Files skipped from review due to trivial changes (1)
- go.mod
Authentik's issuer URL includes a trailing slash; trimming it caused a mismatch between the URL passed to go-oidc and the issuer returned by the provider's discovery document.
- Add structured logging to OIDC callback error paths - Check file availability before incrementing share link use count - Use file::memory:?cache=shared DSN in share handler tests
agjmills
force-pushed
the
fix/oidc-issuer-trailing-slash
branch
from
8186c37 to
7618c43
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
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
strings.TrimRightstripping of trailing slash from the OIDC issuer URL before discoveryTest plan
OIDC provider initialisedlogSummary by CodeRabbit
New Features
UI
Documentation
Tests
UX