Auth PURGE requests #16
Conversation
WalkthroughThis update introduces stricter authentication for PURGE requests by requiring a valid token, expands feed URL matching to allow optional trailing slashes and query parameters, and updates related tests and documentation. Benchmarking and acceptance testing commands are restructured for clarity and consistency, with environment variables and secret handling improved across workflows and scripts. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Varnish
participant Backend
Note over Client,Varnish: PURGE Request Flow with Token
Client->>Varnish: PURGE /some/url with Purge-Token header
alt Purge-Token matches PURGE_TOKEN env
Varnish->>Varnish: Proceed with purge
Varnish-->>Client: 200 OK (purged)
else Purge-Token missing or invalid
Varnish-->>Client: 401 Unauthorized ("Invalid PURGE token")
end
sequenceDiagram
participant Client
participant Varnish
participant Feeds Backend
Note over Client,Varnish: Feed URL Normalization
Client->>Varnish: GET /podcast/feed/ or /podcast/feed?x=1
Varnish->>Varnish: Regex match & normalize to /podcast.xml
Varnish->>Feeds Backend: GET /podcast.xml
Feeds Backend-->>Varnish: Feed XML
Varnish-->>Client: Feed XML response
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (13)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (8)
🧰 Additional context used🪛 golangci-lint (1.64.8)dagger/main.go106-106: undefined: dag (typecheck) ⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (10)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai generate sequence diagram |
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
Sequence Diagram for PRsequenceDiagram
participant Developer
participant GitHub
participant Reviewer
participant CI
Developer->>GitHub: Submit PR
GitHub->>CI: Trigger build
CI->>GitHub: Report status
GitHub->>Reviewer: Notify for review
Reviewer->>GitHub: Approve/Request changes
GitHub->>Developer: Notify result
|
✅ Actions performedSequence diagram generated. |
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
justfile (1)
91-97: 🛠️ Refactor suggestion
deployrunspublishtwice – wastes CI minutes
deployfirst invokes@just publishand then calls$(just publish …)again
to capture the image reference, so the image is built & pushed twice.deploy tag=_DEFAULT_TAG: - @just publish {{ tag }} - @just dagger --mod={{ DAGGER_FLY_MODULE }} call \ - ... - --image=$(just publish {{ tag }}) + IMAGE=$(@just publish {{ tag }}) \ + && @just dagger --mod={{ DAGGER_FLY_MODULE }} call \ + ... \ + --image=$IMAGE
♻️ Duplicate comments (1)
test/vtc/feeds.vtc (1)
296-308: Test regex mirrors production bug – keep in sync after fixThe test also uses the duplicate-group pattern for
/feed.
Once you drop the second(\?.*)?in the VCL, update the assertion here to prevent false positives.
🧹 Nitpick comments (4)
test/vtc/purge.vtc (1)
10-17: Return 403 for bad token instead of 401.401 is intended for unauthenticated clients and invites an
WWW-Authenticateheader. Here the client is authenticated (they sent a token) but not authorized. 403 (“Forbidden”) is semantically closer and avoids misleading tooling.- return(synth(401, "Invalid PURGE token")); + return(synth(403, "Invalid PURGE token"));Update the expectations for the first two client blocks accordingly.
README.md (1)
30-36: Stylistic: avoid repetitive sentence startsThree consecutive bullets begin with “How do we …”. Re-phrasing makes the list easier to scan.
- - How do we make this friendlier to new users? - - How do we make this friendlier to contributions? - - How easy is this to use as your own deployment? + - Friendliness to new users + - Friendliness to contributors + - Ease of running your own deployment🧰 Tools
🪛 LanguageTool
[style] ~36-~36: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...e this friendlier to contributions? - How easy is this to use as your own deploym...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
container.justfile (1)
49-51:PURGE_TOKENmay be empty – tests will silently skip the headerHurl will still send
Purge-Token:with an empty value, so the VCL sees a header present but blank and rejects the request (401).
Add a guard that fails fast when the variable isn’t exported:--variable purge_token=$PURGE_TOKEN \ +{{ if ! ${PURGE_TOKEN:+x} }} && { echo "❌ PURGE_TOKEN not set"; exit 1; } || true +--variable purge_token=$PURGE_TOKEN \test/vtc/feeds.vtc (1)
11-274: High maintenance cost – consider parameterised VTC testsThe file is ~800 lines of near-identical blocks. GNU make style loops or the
foreachfacility in VTC can generate these cases programmatically and keep the spec readable.No action required for this PR, but worth tracking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/_github.yml(0 hunks).github/workflows/_namespace.yml(0 hunks)README.md(4 hunks)container.justfile(1 hunks)dagger/main.go(3 hunks)justfile(4 hunks)test/acceptance/pipedream/assets.hurl(2 hunks)test/acceptance/pipedream/feed.hurl(2 hunks)test/acceptance/pipedream/homepage.hurl(3 hunks)test/vtc/feeds.vtc(4 hunks)test/vtc/purge.vtc(1 hunks)varnish/pipedream.changelog.com.vcl(1 hunks)
💤 Files with no reviewable changes (2)
- .github/workflows/_namespace.yml
- .github/workflows/_github.yml
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~36-~36: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...e this friendlier to contributions? - How easy is this to use as your own deploym...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: on-namespace / run
🔇 Additional comments (2)
test/acceptance/pipedream/homepage.hurl (1)
32-34:⚠️ Potential issueAvoid sending
Purge-Tokenon plain GET.Same concern—limit the token to PURGE operations.
-GET {{host}}?purge=true -Purge-Token: {{purge_token}} +GET {{host}}?purge=trueRemoves unnecessary exposure of credentials.
Likely an incorrect or invalid review comment.
varnish/pipedream.changelog.com.vcl (1)
196-199: Greedy.*in/feeds/.*may swallow?breaking the rewriteBecause
.*is greedy it consumes the query-string, causing the subsequent
regsub()to append.xmlafter the entire URL – result:
/feeds/foo?bar.xmlinstead of/feeds/foo.xml?bar.Safer pattern:
-} else if (req.url ~ "^/feeds/.*(\?.*)?$") { +} else if (req.url ~ "^/feeds/[^?]*(\?.*)?$") {The negative-class prevents matching the
?.
c13e952 to
2e91499
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
dagger/main.go (1)
336-346: Container rebuilt multiple times & token always forwarded to tests.The same issues identified in past review comments still exist:
m.Test(ctx)is called multiple times, rebuilding the same container- The purge token is always passed even when empty, sending an empty
Purge-TokenheaderApply the same fix as previously suggested:
- var purgeToken string - if m.VarnishPurgeToken != nil { - purgeToken, err = m.VarnishPurgeToken.Plaintext(ctx) - if err != nil { - panic(err) - } - } - - return m.Test(ctx). + testContainer := m.Test(ctx) + var purgeToken string + if m.VarnishPurgeToken != nil { + purgeToken, err = m.VarnishPurgeToken.Plaintext(ctx) + if err != nil { + panic(err) + } + } + + args := []string{"just", "test-acceptance-local", "--variable", "host=http://pipely:9000"} + if purgeToken != "" { + args = append(args, "--variable", "purge_token="+purgeToken) + } + + return testContainer. WithServiceBinding("pipely", pipely). - WithExec([]string{"just", "test-acceptance-local", "--variable", "host=http://pipely:9000", "--variable", "purge_token=" + purgeToken}) + WithExec(args)
🧹 Nitpick comments (1)
README.md (1)
34-36: Consider varying sentence beginnings for better readability.Three consecutive questions begin with "How", which slightly affects readability.
- How do we make this friendlier to new users? - How do we make this friendlier to contributions? - - How easy is this to use as your own deployment? + - Is this easy to use as your own deployment?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/_github.yml(0 hunks).github/workflows/_namespace.yml(0 hunks)README.md(4 hunks)container.justfile(1 hunks)dagger/main.go(5 hunks)justfile(4 hunks)test/acceptance/pipedream/assets.hurl(2 hunks)test/acceptance/pipedream/feed.hurl(2 hunks)test/acceptance/pipedream/homepage.hurl(3 hunks)test/vtc/feeds.vtc(4 hunks)test/vtc/purge.vtc(1 hunks)varnish/pipedream.changelog.com.vcl(1 hunks)
💤 Files with no reviewable changes (2)
- .github/workflows/_github.yml
- .github/workflows/_namespace.yml
🚧 Files skipped from review as they are similar to previous changes (5)
- test/vtc/purge.vtc
- test/acceptance/pipedream/assets.hurl
- varnish/pipedream.changelog.com.vcl
- test/acceptance/pipedream/homepage.hurl
- test/acceptance/pipedream/feed.hurl
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~36-~36: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...e this friendlier to contributions? - How easy is this to use as your own deploym...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 golangci-lint (1.64.8)
dagger/main.go
104-104: undefined: dag
(typecheck)
🔇 Additional comments (9)
test/vtc/feeds.vtc (1)
11-350: Excellent test coverage for feed URL variations.The comprehensive test coverage for trailing slashes, query parameters, and all feed endpoints ensures robust URL normalization functionality. The systematic approach with consistent patterns across all feed types is well-implemented.
README.md (1)
26-26: Documentation accurately reflects completed PURGE authentication.The checklist update from partial to complete status correctly reflects the implementation of PURGE token authentication.
dagger/main.go (1)
118-121: The conditional PURGE_TOKEN handling looks correct.The implementation properly checks if
VarnishPurgeTokenis not nil before setting the environment variable, which maintains a clear contract between presence and absence of authentication.container.justfile (2)
49-49: Proper environment variable handling for purge token.The addition of
purge_token=${PURGE_TOKEN:-""}correctly handles the optional purge token with a sensible default, aligning with the authentication requirements.
62-95: Well-organized benchmark targets with clear naming convention.The systematic reorganization of benchmark targets with numeric suffixes (1-6) and explicit environment labels (origin, Fastly, Bunny, Pipedream, local, TLS proxy) significantly improves clarity and maintainability.
justfile (4)
25-29: Improved local testing with explicit purge token handling.The explicit multiline command structure with
PURGE_TOKEN="test-acceptance-local"provides clear, testable authentication for local development.
33-41: Secure credential management for production testing.The use of 1Password CLI (
op read) for secure purge token retrieval follows security best practices and prevents credential exposure in the codebase.
91-96: Improved deploy workflow with dependency on publish.The refactored deploy target that internally calls publish and uses its output ensures consistency and eliminates the need for separate publish commands in CI workflows.
106-109: Enhanced secrets management with multiple credentials.The consolidated secrets command efficiently sets both
HONEYCOMB_API_KEYandPURGE_TOKENin a single operation, improving deployment efficiency.
2e91499 to
f82b321
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
dagger/main.go (1)
329-346: Unresolved issues from previous review remain.The TestAcceptance method still has the same problems flagged in past reviews:
m.Test(ctx)is called twice, causing redundant container rebuilds- The
purge_tokenvariable is always passed even when empty, potentially leaking an empty tokenApply the suggested fix from the previous review:
- pipely, err := m.Test(ctx). + testContainer := m.Test(ctx) + pipely, err := testContainer. AsService(dagger.ContainerAsServiceOpts{UseEntrypoint: true}). Start(ctx) if err != nil { panic(err) } var purgeToken string if m.VarnishPurgeToken != nil { purgeToken, err = m.VarnishPurgeToken.Plaintext(ctx) if err != nil { panic(err) } } - return m.Test(ctx). + return testContainer. WithServiceBinding("pipely", pipely). - WithExec([]string{"just", "test-acceptance-local", "--variable", "host=http://pipely:9000", "--variable", "purge_token=" + purgeToken}) + WithExec(append( + []string{"just", "test-acceptance-local", "--variable", "host=http://pipely:9000"}, + func() []string { + if purgeToken != "" { + return []string{"--variable", "purge_token=" + purgeToken} + } + return nil + }()...))
🧹 Nitpick comments (1)
README.md (1)
30-36: Comprehensive review checklist improves contributor guidance.The expanded checklist with detailed questions about benchmarking, VCL efficiency, functionality, and user-friendliness provides excellent guidance for contributors and reviewers.
Consider varying the sentence beginnings to improve readability:
- How do we make this friendlier to new users? - How do we make this friendlier to contributions? - How easy is this to use as your own deployment? + How can we make this friendlier to new users? + What would make this more contribution-friendly? + How easy is this to use as your own deployment?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/_github.yml(0 hunks).github/workflows/_namespace.yml(0 hunks)README.md(4 hunks)container.justfile(1 hunks)dagger/main.go(5 hunks)justfile(4 hunks)test/acceptance/pipedream/assets.hurl(2 hunks)test/acceptance/pipedream/feed.hurl(2 hunks)test/acceptance/pipedream/homepage.hurl(3 hunks)test/vtc/feeds.vtc(4 hunks)test/vtc/purge.vtc(1 hunks)varnish/pipedream.changelog.com.vcl(1 hunks)
💤 Files with no reviewable changes (2)
- .github/workflows/_namespace.yml
- .github/workflows/_github.yml
🚧 Files skipped from review as they are similar to previous changes (5)
- test/vtc/purge.vtc
- varnish/pipedream.changelog.com.vcl
- test/acceptance/pipedream/feed.hurl
- test/acceptance/pipedream/homepage.hurl
- justfile
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~36-~36: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...e this friendlier to contributions? - How easy is this to use as your own deploym...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 golangci-lint (1.64.8)
dagger/main.go
104-104: undefined: dag
(typecheck)
🔇 Additional comments (9)
test/vtc/feeds.vtc (3)
10-275: Excellent comprehensive test coverage for feed URL variants.The mock server setup thoroughly covers all feed endpoints with their URL variants (base, trailing slash, and query parameters). This comprehensive approach ensures robust testing of the URL routing logic.
296-350: VCL configuration correctly implements feed URL routing.The regex patterns properly handle optional trailing slashes and query parameters with single capture groups. The duplicate regex issue from previous reviews has been resolved. The URL rewriting logic consistently transforms feed URLs to their corresponding .xml endpoints.
385-799: Comprehensive client test coverage validates all feed URL routing scenarios.The test cases thoroughly validate all feed endpoints with their URL variants. The systematic approach of testing base URLs, trailing slashes, and query parameters ensures robust validation of the routing logic. The consistent test structure enhances maintainability.
dagger/main.go (1)
118-121: LGTM - Conditional environment variable setting.The conditional logic correctly addresses the previous review concern by only setting the PURGE_TOKEN environment variable when a token is provided, maintaining a clear contract between presence and absence of the variable.
test/acceptance/pipedream/assets.hurl (2)
12-18: LGTM - Proper PURGE authentication.The PURGE request correctly includes the
Purge-Tokenheader for authentication and updates the assertions to validate the synthetic response. Good that the token is only included on PURGE requests and not leaked on GET requests.
2-2: Query parameter addition enhances test coverage.Adding the
?hurl=trueparameter to the GET request improves test differentiation and coverage.README.md (1)
26-26: LGTM - Accurately reflects completed feature.The checklist update correctly marks the PURGE authentication feature as completed, aligning with the PR implementation.
container.justfile (2)
49-49: LGTM - Proper purge token integration.The purge token variable correctly sources from the environment with an appropriate empty string fallback for optional authentication scenarios.
57-95: Excellent benchmark organization improvement.The restructured benchmark targets with explicit numbering, clear descriptions, and logical grouping (app vs feed, different environments) significantly improve usability and maintainability.
86da29a to
f1a8d83
Compare
So that we don't allow anyone to PURGE URLs. `purge-token` request header is required if the instance has been configured with `PURGE_TOKEN` secret (exposed as an environment variable). And a few other improvements: - add `PURGE` VTC & acceptance tests - handle URL parameters for all feed redirects - add extra tests to cover feed URL variations - wrap long commands in the `justfile` so that they are easier to read - separate the benchmark just commands into app & feed (inside the debug container) & capture the intended run order - add a few ideas for all contributors review - upgrade Dagger to v0.18.12 (latest release) Required by: - thechangelog/changelog.com#549 Signed-off-by: Gerhard Lazu <gerhard@changelog.com>
f1a8d83 to
45df30a
Compare
So that we don't allow anyone to PURGE URLs.
purge-tokenrequest header is required if the instance has been configured withPURGE_TOKENsecret (exposed as an environment variable).A few other improvements:
PURGEVTC & acceptance testsjustfileso that they are easier to readRequired by:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores