feat(badges): accept URL-safe base64 (base64url) badge artifacts - #576
feat(badges): accept URL-safe base64 (base64url) badge artifacts#576smarcet wants to merge 4 commits into
Conversation
Badge QR artifacts travel base64-encoded as a URL path segment
(GET /summits/{id}/badge/{artifact}/validate). The standard alphabet
includes '/', and Laravel rawurldecodes the path before route matching
(UriValidator) with {badge} compiled to [^/]+ - so any artifact whose
wrapper contains '/' can never match the route and 404s with the HTML
error page (ClickUp 86bb7wfgw). Accepting the RFC 4648 section 5
URL-safe alphabet lets callers keep the artifact in the path with no
percent-encoding tricks.
- looksLikeBase64: widen the alphabet to also accept '-' and '_'
- tryBase64Decode: normalize (strtr '-_' -> '+/') before the strict
base64_decode, which rejects the URL-safe alphabet
Additive and backwards-compatible: standard base64 decodes byte
identical; the sole caller is SummitAttendeeBadge::decodeQRCodeFor, so
badge-scans POST, checkin and validateBadge gain it uniformly and
existing clients are unaffected.
Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 49 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Base64 utility now accepts standard and URL-safe alphabets. It validates padding, adds omitted padding, and normalizes URL-safe characters before strict decoding. PHPUnit tests cover valid and invalid inputs. ChangesURL-safe Base64 support
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-576/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Utils/Base64.php`:
- Around line 20-22: Update the Base64 validation near the existing preg_match
and length check to reject padding-only and partially padded inputs such as “==”
and “A=”. Validate the unpadded data length and allow either no padding or
exactly the RFC 4648 padding required to reach a multiple of four, while
preserving standard and URL-safe alphabets. Add regression tests covering
padding-only and partially padded inputs.
🪄 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 Plus
Run ID: badd35d8-20a2-4b60-ab4f-e1d44e4ab976
📒 Files selected for processing (2)
app/Utils/Base64.phptests/Base64Test.php
Per CodeRabbit review on PR #576: the sniff accepted padding-only and partially padded inputs ('==', 'A=', 'QUFB==') and silently repaired under-padded ones ('QQ=' decoded as 'QQ=='). Padding may now be omitted entirely (padBase64 adds it) or must be exactly what the unpadded data length requires (RFC 4648); the strict decode already rejected these one step later, so caller-visible behavior only changes for the silently repaired case, which is now rejected. Co-Authored-By: Claude <noreply@anthropic.com>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-576/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Pull request overview
This PR updates the shared App\Utils\Base64 helper to accept URL-safe base64 (base64url) so badge QR artifacts can be transported safely as URL path segments without route-matching failures caused by / after Laravel’s raw URL decoding.
Changes:
- Expand base64 validation to accept
-and_(base64url alphabet) and enforce stricter, RFC-consistent padding rules. - Normalize base64url strings (
-_/→+/) before strict decoding. - Add focused unit tests covering standard/base64url equivalence, padding omission tolerance, and invalid input rejection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/Utils/Base64.php | Accept base64url characters and normalize before strict decode; tighten padding validation rules. |
| tests/Base64Test.php | Add new unit tests validating standard vs URL-safe decoding, padding behavior, and invalid input rejection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Per Copilot review on PR #576: base64_decode('', true) succeeds with '' so tryBase64Decode('') returned '' instead of null, and under-padded input ('QQ=') was still silently repaired at the decode level even though looksLikeBase64 now rejects it. Short-circuit tryBase64Decode with looksLikeBase64 so the pair holds one invariant: whatever the sniff rejects, the decode returns null for. Behavior-neutral for the sole caller (decodeQRCodeFor only decodes after the sniff passes). Co-Authored-By: Claude <noreply@anthropic.com>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-576/ This page is automatically updated on each push to this PR. |
…e param The swagger published from these annotations described the artifact as standard base64 only; since this branch the transport wrapper may also use the RFC 4648 URL-safe alphabet. Co-Authored-By: Claude <noreply@anthropic.com>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-576/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
app/Utils/Base64.php:28
- The padding check currently treats an unpadded length that’s already a multiple of 4 as requiring 4 padding chars (because
4 - 0), which is incorrect per RFC 4648. It doesn’t manifest today due to the{0,2}regex cap, but it makes the logic misleading and fragile if the regex ever changes.
return $paddingLength === 0 || $paddingLength === (4 - $remainder);
ref: https://app.clickup.com/t/9014802374/86bb7wfgw
What
Widen
App\Utils\Base64to also accept the URL-safe base64 alphabet (RFC 4648 §5:-for+,_for/), so badge QR artifacts can travel as a URL path segment with no percent-encoding tricks.Why
GET /summits/{id}/badge/{artifact}/validatetakes the artifact as a path segment, and Laravel rawurldecodes the path before route matching (Illuminate/Routing/Matching/UriValidator.php) with{badge}compiled to[^/]+— so any artifact whose base64 wrapper contains/(near-certain for encrypted badges) can never match the route and 404s, no matter how the client percent-encodes it. Full analysis on ClickUp 86bb7wfgw.Accepting base64url keeps the existing route and payload untouched (one route, one payload — D26): the path segment stays in
[A-Za-z0-9_-]plus=padding, all legal in a path segment.How
looksLikeBase64: alphabet regex also accepts-and_tryBase64Decode: normalize (strtr('-_', '+/')) before the strictbase64_decode, which rejects the URL-safe alphabet — widening the regex alone is not enoughAdditive and backwards-compatible: standard base64 decodes byte-identical; the sole caller is
SummitAttendeeBadge::decodeQRCodeFor, so badge-scans POST and validateBadge gain it uniformly and existing clients (scanbadgeapp, sponsor lead flows) are unaffected. Checkin is out of scope either way:AttendeeService::doCheckInparses the raw QR directly and never unwrapped base64. Badge QR emission does not change — the inner{hex-IV}{base64(ciphertext)}layer decoded byAES::decryptstays standard; only the per-request transport wrapper may now be url-safe.Tests
tests/Base64Test.php(new, pure unit — no app boot): standard alphabet regression, url-safe acceptance, spelling equivalence, padding tolerance, garbage rejection.OK (5 tests, 11 assertions).Deployment
Deploy this before attendee-networking-api's matching change (its
resolve_badgere-wraps artifacts url-safe and depends on this decode).Summary by CodeRabbit
Summary by CodeRabbit
New Features
-and_.Bug Fixes