Add anonymous URL ownership claim via manage_token#134
Open
revanthlol wants to merge 5 commits intospoo-me:mainfrom
Open
Add anonymous URL ownership claim via manage_token#134revanthlol wants to merge 5 commits intospoo-me:mainfrom
revanthlol wants to merge 5 commits intospoo-me:mainfrom
Conversation
Anonymous users receive a one-time manage_token when shortening a URL. Token is stored in recentURLs localStorage alongside the alias. After signing in, all unclaimed URLs are automatically claimed via a post-login sweep. Users can also trigger the flow manually from the result page or the recent links list on the homepage. - Add manage_token field to UrlV2Doc (hashed, never stored plaintext) - Add claim_by_manage_token() to UrlRepository (atomic, single update_one) - Add POST /api/v1/claim endpoint with JWT auth required - Expose raw token once in shorten response for anonymous users only - Store token inside recentURLs[] objects in localStorage - Show Unclaimed badge + Claim All button in recent links UI - Show token banner on result page with copy + claim flow - Auto-sweep and claim all stored tokens on login/register - Add integration tests for claim endpoint and token lifecycle
Contributor
Reviewer's GuideImplements anonymous URL ownership claiming via a one-time manage_token: backend now generates and stores a hashed token for anonymous shortens, exposes the raw token once in the shorten response, adds an atomic /api/v1/claim endpoint to transfer ownership, and frontend wiring to store tokens in localStorage, surface ‘Unclaimed’ UI, and auto-claim after authentication, along with integration tests. Sequence diagram for anonymous URL creation with manage_tokensequenceDiagram
actor User
participant Browser
participant ApiV1Shorten
participant UrlService
participant UrlRepository
participant MongoDB
User->>Browser: Submit URL to shorten
Browser->>ApiV1Shorten: POST /api/v1/shorten (CreateUrlRequest)
ApiV1Shorten->>UrlService: create(request, owner_id=None, client_ip)
activate UrlService
UrlService->>UrlService: validate request, generate alias, build doc
UrlService->>UrlService: raw_token = token_urlsafe(32)
UrlService->>UrlService: doc.manage_token = hash_token(raw_token)
UrlService->>UrlRepository: insert(doc)
activate UrlRepository
UrlRepository->>MongoDB: insert_one(doc)
MongoDB-->>UrlRepository: inserted_id
deactivate UrlRepository
UrlService-->>ApiV1Shorten: UrlV2Doc, raw_token
deactivate UrlService
ApiV1Shorten-->>Browser: UrlResponse(alias, ..., manage_token=raw_token)
Browser->>Browser: Parse response JSON
Browser->>Browser: Update localStorage.recentURLs with { alias, manage_token }
Browser-->>User: Redirect to /result/alias and show token banner
Sequence diagram for claiming anonymous URLs via sweepAndClaimTokenssequenceDiagram
actor User
participant Browser
participant AuthJS
participant ApiV1Claim
participant UrlService
participant UrlRepository
participant MongoDB
User->>Browser: Complete login or registration
Browser->>AuthJS: submitAuth success handler
activate AuthJS
AuthJS->>AuthJS: sweepAndClaimTokens()
AuthJS->>AuthJS: Load recentURLs from localStorage
loop For each item with manage_token
AuthJS->>ApiV1Claim: POST /api/v1/claim { alias, manage_token }
activate ApiV1Claim
ApiV1Claim->>UrlService: claim_url(alias, raw_token, new_owner_id)
activate UrlService
UrlService->>UrlService: token_hash = hash_token(raw_token)
UrlService->>UrlRepository: claim_by_manage_token(alias, token_hash, new_owner_id)
activate UrlRepository
UrlRepository->>MongoDB: update_one(filter: alias, owner_id=ANONYMOUS_OWNER_ID, manage_token=token_hash, set owner_id, manage_token=None, updated_at)
MongoDB-->>UrlRepository: UpdateResult(modified_count)
UrlRepository-->>UrlService: modified_count > 0
deactivate UrlRepository
UrlService-->>ApiV1Claim: claimed True or False
deactivate UrlService
alt claimed is True
ApiV1Claim-->>AuthJS: 200 ClaimUrlResponse(success=True)
else claimed is False
ApiV1Claim-->>AuthJS: 403 HTTPException
end
deactivate ApiV1Claim
alt res.ok or status in {403, 404, 409}
AuthJS->>AuthJS: item.manage_token = null
else Server error
AuthJS->>AuthJS: keep token for retry
end
end
AuthJS->>AuthJS: localStorage.recentURLs updated (cleared tokens)
AuthJS-->>Browser: Redirect to spoo_claim_redirect or dashboard
deactivate AuthJS
Updated class diagram for URL and claim models and servicesclassDiagram
class UrlV2Doc {
ObjectId id
string alias
string long_url
ObjectId owner_id
datetime created_at
int total_clicks
datetime last_click
datetime updated_at
string manage_token
}
class UrlResponse {
string alias
string long_url
int ttl
int short_url_clicks
float created_at
string status
bool private_stats
string manage_token
}
class UrlService {
+create(request, owner_id, client_ip) tuple_UrlV2Doc_OptionalStr
+update(alias, request, owner_id) UrlV2Doc
+list_by_owner(owner_id, page, page_size, sort_by, sort_order) dict
+claim_url(alias, raw_token, new_owner_id) bool
-_dispatch(short_code) tuple_OptionalUrlCacheData_str
}
class UrlRepository {
+insert(doc) ObjectId
+check_stats_privacy(alias) dict
+claim_by_manage_token(alias, token_hash, new_owner_id) bool
}
class ClaimUrlRequest {
string alias
string manage_token
}
class ClaimUrlResponse {
bool success
string message
}
UrlService --> UrlRepository : uses
UrlService --> UrlV2Doc : creates_and_returns
UrlService --> ClaimUrlResponse : returned_via_api
ClaimUrlRequest --> UrlService : input_to_claim_url
UrlResponse --> UrlV2Doc : serialized_view
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughAdds a one-time manage token flow for anonymous shortened URLs: tokens are generated and hashed at creation, returned once to the client, stored in recent-URLs localStorage, and can be claimed via a new authenticated Changes
Sequence Diagram(s)sequenceDiagram
actor Browser
participant Shorten as /api/v1/shorten
participant Service as UrlService
participant Repo as UrlRepository
participant DB as MongoDB
Browser->>Shorten: POST create (unauthenticated)
Shorten->>Service: create(owner_id=None,...)
Service->>Service: generate raw_token & hash it
Service->>Repo: insert doc with manage_token hash
Repo->>DB: insertOne(doc)
DB-->>Repo: success
Repo-->>Service: created doc
Service-->>Shorten: (doc, raw_token)
Shorten-->>Browser: UrlResponse { ..., manage_token: raw_token }
Browser->>Browser: store {alias, manage_token} in localStorage
sequenceDiagram
actor Browser
participant Auth as Auth endpoint
participant Sweep as sweepAndClaimTokens()
participant Claim as /api/v1/claim
participant Service as UrlService
participant Repo as UrlRepository
participant DB as MongoDB
Browser->>Auth: POST login
Auth-->>Browser: 200 OK
Browser->>Sweep: sweepAndClaimTokens()
Sweep->>Sweep: read recentURLs + find entries with manage_token
loop per entry
Sweep->>Claim: POST {alias, manage_token}
Claim->>Service: claim_url(alias, raw_token, new_owner_id)
Service->>Service: hash raw_token
Service->>Repo: claim_by_manage_token(alias, token_hash, new_owner_id)
Repo->>DB: updateOne(match {alias, owner_id=ANONYMOUS, manage_token=hash})
DB-->>Repo: modified_count
Repo-->>Service: True/False
Service-->>Claim: Bool result
Claim-->>Sweep: 200 or 403
Sweep->>Sweep: clear token from localStorage on success/known failures
end
Sweep->>Browser: navigate to stored redirect
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 security issues, 1 other issue, and left some high level feedback:
Security issues:
- User controlled data in methods like
innerHTML,outerHTMLordocument.writeis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in a
wrapper.innerHTMLis an anti-pattern that can lead to XSS vulnerabilities (link)
General comments:
- In
routes/api_v1/claim.py, the docstring and inline comment say the endpoint returns 404/409 for specific failure modes, but the implementation always returns 403 on failure; either update the behavior to match the documented statuses or simplify the docs to match the single 403 code actually returned. - The
claim_urlroute currently accepts an arbitrarymanage_tokenstring; consider adding basic validation constraints (e.g., min_length / max_length consistent withsecrets.token_urlsafe(32)) onClaimUrlRequest.manage_tokento fail clearly on obviously malformed input before hitting the service layer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `routes/api_v1/claim.py`, the docstring and inline comment say the endpoint returns 404/409 for specific failure modes, but the implementation always returns 403 on failure; either update the behavior to match the documented statuses or simplify the docs to match the single 403 code actually returned.
- The `claim_url` route currently accepts an arbitrary `manage_token` string; consider adding basic validation constraints (e.g., min_length / max_length consistent with `secrets.token_urlsafe(32)`) on `ClaimUrlRequest.manage_token` to fail clearly on obviously malformed input before hitting the service layer.
## Individual Comments
### Comment 1
<location path="static/js/result-script.js" line_range="83-90" />
<code_context>
+ banner.style.display = 'flex';
+})();
+
+function copyManageToken() {
+ const valueEl = document.getElementById('claim-token-value');
+ if (!valueEl) return;
+ navigator.clipboard.writeText(valueEl.textContent.trim()).then(() => {
+ const btn = document.getElementById('claim-token-copy-btn');
+ if (btn) { btn.textContent = 'Copied!'; setTimeout(() => { btn.textContent = 'Copy'; }, 2000); }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Guard against navigator.clipboard availability and insecure-context failures.
This directly calls `navigator.clipboard.writeText`, which may be unavailable or reject in older browsers or non-secure contexts. Consider feature-detecting `navigator.clipboard` and either falling back to a selection-based copy or returning early to avoid runtime errors or silent failures.
```suggestion
function copyManageToken() {
const valueEl = document.getElementById('claim-token-value');
if (!valueEl) return;
const text = valueEl.textContent.trim();
const btn = document.getElementById('claim-token-copy-btn');
const markCopied = () => {
if (!btn) return;
btn.textContent = 'Copied!';
setTimeout(() => { btn.textContent = 'Copy'; }, 2000);
};
// Prefer modern async clipboard API in secure contexts
if (navigator.clipboard && window.isSecureContext) {
navigator.clipboard.writeText(text)
.then(markCopied)
.catch((err) => {
console.error('Failed to copy claim token via Clipboard API:', err);
});
return;
}
// Fallback: selection-based copy for older browsers / non-secure contexts
try {
const selection = window.getSelection();
if (!selection) return;
const range = document.createRange();
range.selectNodeContents(valueEl);
selection.removeAllRanges();
selection.addRange(range);
const successful = typeof document.execCommand === 'function' && document.execCommand('copy');
selection.removeAllRanges();
if (successful) {
markCopied();
} else {
console.warn('execCommand("copy") did not succeed when copying claim token.');
}
} catch (err) {
console.error('Error attempting fallback copy for claim token:', err);
}
}
```
</issue_to_address>
### Comment 2
<location path="static/js/index-recent-links.js" line_range="84-90" />
<code_context>
wrapper.innerHTML = `
<div class="section-1">
<div class="left-section">
<span class="short-url">
<a href="/${alias}" target="_blank">${shortUrl.replace(/^https?:\/\//, '')}</a>
</span>
${hasToken ? '<span class="unclaimed-badge">Unclaimed</span>' : ''}
</div>
</div>
<div class="section-2">
<div class="button-container">
<button class="copy-button" data-url="${shortUrl}">Copy</button>
<button class="edit-button" data-alias="${alias}">Edit</button>
<button class="stats-button">Stats</button>
</div>
</div>
`;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-document-method):** User controlled data in methods like `innerHTML`, `outerHTML` or `document.write` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 3
<location path="static/js/index-recent-links.js" line_range="84-90" />
<code_context>
wrapper.innerHTML = `
<div class="section-1">
<div class="left-section">
<span class="short-url">
<a href="/${alias}" target="_blank">${shortUrl.replace(/^https?:\/\//, '')}</a>
</span>
${hasToken ? '<span class="unclaimed-badge">Unclaimed</span>' : ''}
</div>
</div>
<div class="section-2">
<div class="button-container">
<button class="copy-button" data-url="${shortUrl}">Copy</button>
<button class="edit-button" data-alias="${alias}">Edit</button>
<button class="stats-button">Stats</button>
</div>
</div>
`;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-innerhtml):** User controlled data in a `wrapper.innerHTML` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+83
to
+90
| function copyManageToken() { | ||
| const valueEl = document.getElementById('claim-token-value'); | ||
| if (!valueEl) return; | ||
| navigator.clipboard.writeText(valueEl.textContent.trim()).then(() => { | ||
| const btn = document.getElementById('claim-token-copy-btn'); | ||
| if (btn) { btn.textContent = 'Copied!'; setTimeout(() => { btn.textContent = 'Copy'; }, 2000); } | ||
| }).catch(console.error); | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Guard against navigator.clipboard availability and insecure-context failures.
This directly calls navigator.clipboard.writeText, which may be unavailable or reject in older browsers or non-secure contexts. Consider feature-detecting navigator.clipboard and either falling back to a selection-based copy or returning early to avoid runtime errors or silent failures.
Suggested change
| function copyManageToken() { | |
| const valueEl = document.getElementById('claim-token-value'); | |
| if (!valueEl) return; | |
| navigator.clipboard.writeText(valueEl.textContent.trim()).then(() => { | |
| const btn = document.getElementById('claim-token-copy-btn'); | |
| if (btn) { btn.textContent = 'Copied!'; setTimeout(() => { btn.textContent = 'Copy'; }, 2000); } | |
| }).catch(console.error); | |
| } | |
| function copyManageToken() { | |
| const valueEl = document.getElementById('claim-token-value'); | |
| if (!valueEl) return; | |
| const text = valueEl.textContent.trim(); | |
| const btn = document.getElementById('claim-token-copy-btn'); | |
| const markCopied = () => { | |
| if (!btn) return; | |
| btn.textContent = 'Copied!'; | |
| setTimeout(() => { btn.textContent = 'Copy'; }, 2000); | |
| }; | |
| // Prefer modern async clipboard API in secure contexts | |
| if (navigator.clipboard && window.isSecureContext) { | |
| navigator.clipboard.writeText(text) | |
| .then(markCopied) | |
| .catch((err) => { | |
| console.error('Failed to copy claim token via Clipboard API:', err); | |
| }); | |
| return; | |
| } | |
| // Fallback: selection-based copy for older browsers / non-secure contexts | |
| try { | |
| const selection = window.getSelection(); | |
| if (!selection) return; | |
| const range = document.createRange(); | |
| range.selectNodeContents(valueEl); | |
| selection.removeAllRanges(); | |
| selection.addRange(range); | |
| const successful = typeof document.execCommand === 'function' && document.execCommand('copy'); | |
| selection.removeAllRanges(); | |
| if (successful) { | |
| markCopied(); | |
| } else { | |
| console.warn('execCommand("copy") did not succeed when copying claim token.'); | |
| } | |
| } catch (err) { | |
| console.error('Error attempting fallback copy for claim token:', err); | |
| } | |
| } |
There was a problem hiding this comment.
Pull request overview
Adds a one-time “manage token” flow so anonymous users can later claim ownership of URLs they created before signing up/logging in, implemented end-to-end across the API, persistence layer, UI, and tests.
Changes:
- Backend: generate/store hashed
manage_tokenfor anonymous URL creation; add atomicPOST /api/v1/claimto transfer ownership and invalidate the token. - Frontend: persist
{alias, manage_token}inrecentURLs, show “Unclaimed” UI + token banner, and auto-sweep/claim tokens after login/register. - Tests: update shorten mocks for tuple return; add integration tests for claim behavior and token exposure rules.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
services/url_service.py |
Generates manage token for anonymous creates; adds claim_url; changes create() return type to tuple. |
repositories/url_repository.py |
Adds atomic claim_by_manage_token() Mongo update. |
routes/api_v1/claim.py |
New authenticated claim endpoint. |
routes/api_v1/shorten.py |
Unpacks tuple return and exposes manage_token in create response. |
routes/api_v1/__init__.py |
Registers the new claim router. |
schemas/models/url.py |
Adds manage_token field to URL document model. |
schemas/dto/responses/url.py |
Adds manage_token to UrlResponse. |
static/js/index-script.js |
Stores {alias, manage_token} entries in recentURLs. |
static/js/index-recent-links.js |
Renders “Unclaimed” badges and “Sign in to claim all your links” CTA. |
static/js/result-script.js |
Displays token banner and provides copy/claim entry points. |
static/js/auth.js |
Sweeps local tokens and calls /api/v1/claim after auth success; redirects back if requested. |
templates/result.html |
Adds token banner markup; bumps CSS cache-buster. |
templates/index.html |
Bumps JS/CSS cache-busters for updated assets. |
static/css/result.css |
Styles token banner. |
static/css/index.css |
Styles unclaimed badge and claim-all button. |
tests/integration/api_v1/test_claim.py |
New integration tests for claim and token response behavior. |
tests/integration/api_v1/test_shorten.py |
Updates mocks for UrlService.create() tuple return. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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 (4)
schemas/models/url.py (1)
52-52: Keep the stored token hash out of defaultUrlV2Docserialization.
UrlV2Docnow carries a DB-only secret hash, andservices/url_service.py, Line 393 already uses a genericmodel_dump()on this model. Leaving the field serializable by default makes it easy for a future handler or log path to emit something this PR intends to keep internal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@schemas/models/url.py` at line 52, UrlV2Doc currently exposes the DB-only manage_token field by default; mark it non-serializable so model_dump() and other default serializers won't emit it. Update the manage_token declaration on UrlV2Doc to use a Pydantic field with exclusion (e.g., manage_token: Optional[str] = Field(None, exclude=True)) or the equivalent pydantic v2 config so the attribute is omitted from serialization; keep the name manage_token so existing code that accesses the value internally continues to work. Ensure you import Field from pydantic and run tests to confirm places that call model_dump() (e.g., url service code paths that serialize UrlV2Doc) no longer include the token.tests/integration/api_v1/test_claim.py (3)
53-68: Consider consolidating duplicate test logic with parametrization.
test_claim_wrong_tokenandtest_claim_already_claimedhave identical implementation (both mockclaim_urlreturningFalseand assert 403). While documenting distinct semantic cases is valuable, you could use@pytest.mark.parametrizeto reduce duplication while preserving the test case names.♻️ Optional: Consolidate with parametrize
import pytest `@pytest.mark.parametrize`("alias,token,scenario", [ ("testme", "wrong_token", "wrong_token"), ("claimed", "old_token", "already_claimed"), ]) def test_claim_fails_returns_403(self, alias, token, scenario): """Service returns False for wrong token or already-claimed URL.""" user = _make_user() mock_svc = AsyncMock() mock_svc.claim_url = AsyncMock(return_value=False) application = _build_test_app( {get_current_user: lambda: user, get_url_service: lambda: mock_svc} ) with TestClient(application, raise_server_exceptions=False) as client: resp = client.post( "/api/v1/claim", json={"alias": alias, "manage_token": token}, ) assert resp.status_code == 403🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/api_v1/test_claim.py` around lines 53 - 68, Both test_claim_wrong_token and test_claim_already_claimed duplicate the same logic (mocking AsyncMock.claim_url to return False and asserting a 403); replace them with a single parametrized test that calls the same request with different inputs. Use pytest.mark.parametrize to pass alias and manage_token values (e.g., ("testme","wrong_token") and ("claimed","old_token")) into one test function (e.g., test_claim_fails_returns_403) while keeping the same setup: create user via _make_user(), mock_svc with claim_url = AsyncMock(return_value=False), build the app via _build_test_app({get_current_user: ..., get_url_service: ...}), perform the POST to "/api/v1/claim", and assert resp.status_code == 403.
1-1: Docstring doesn't reflect all tests in this file.The docstring says "Tests for POST /api/v1/claim" but the file also includes tests for
/api/v1/shorten(lines 82-115). Consider either updating the docstring to reflect both endpoints or moving the shorten tests to their respective test file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/api_v1/test_claim.py` at line 1, The module-level docstring currently states only "Tests for POST /api/v1/claim" but the file also contains tests for the /api/v1/shorten endpoint (see tests around lines referencing shorten tests), so update the top docstring to accurately reflect both endpoints (e.g., "Tests for POST /api/v1/claim and POST /api/v1/shorten") or move the shorten-related test functions into their dedicated shorten test file; ensure you update any import/fixture references if relocating tests and keep function names like the shorten test functions unchanged.
14-80: Consider adding validation and edge-case tests.The happy path and auth flows are well-covered. For completeness, consider adding tests for:
- Missing or empty
alias/manage_tokenfields (expect 422 validation error)- Service raising an unexpected exception (verify it propagates as 500)
These can be deferred if validation is tested elsewhere.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/api_v1/test_claim.py` around lines 14 - 80, Add two tests to TestClaim: one (e.g., test_claim_validation_errors) that posts to "/api/v1/claim" with missing or empty "alias" and/or "manage_token" and asserts a 422 response (validation error) using _build_test_app with get_current_user returning a valid user and get_url_service a mock; and another (e.g., test_claim_service_exception) that configures mock_svc.claim_url to raise an exception (side_effect=Exception("boom")) and asserts the request to the claim endpoint yields a 500 (or propagates as server error) by using TestClient(application, raise_server_exceptions=False) so you verify the service exception path in the existing TestClaim test class.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@repositories/url_repository.py`:
- Around line 257-263: When claiming ownership in the update that sets
"owner_id" and "manage_token" (the block using new_owner_id), also normalize the
private_stats field so it is never left as None: read the current document's
private_stats and set "$set": {"private_stats": False} if it's None (or set to
the existing value if already a boolean) so UrlV2Doc's boolean invariant is
preserved; update the same $set payload that contains
owner_id/manage_token/updated_at to include the normalized private_stats value.
In `@routes/api_v1/claim.py`:
- Around line 1-63: The file fails Ruff formatting; run an automatic formatter
(ruff format) or apply the same style changes: fix import ordering/spacing,
ensure consistent blank lines around the module docstring and between imports
and definitions, normalize indentation and trailing whitespace in the router and
function definitions (notably around claim_url, ClaimUrlRequest,
ClaimUrlResponse, and the top module docstring), and re-run ruff to confirm the
file is lint/format clean.
- Around line 45-50: Update the docstring for the claim transfer endpoint in
routes/api_v1/claim.py (the function that "Transfer ownership of an anonymously
created URL to your account") to reflect actual behavior: note that all failure
cases return 403 (not distinct 403/409/404) to avoid oracle attacks and that the
manage_token is single-use and invalidated on success; keep the short
description and single-use token line but replace the incorrect "Returns 403 if
the token is wrong, 409 if already claimed, 404 if not found" sentence with a
single statement that failures return 403 for any error condition.
In `@services/url_service.py`:
- Line 169: The UrlService.create signature now returns a tuple (UrlV2Doc,
Optional[str]) but some callers and mocks still expect a single UrlV2Doc; update
every caller and mock to unpack or accept the tuple (doc, raw_token) and to
return the tuple from fakes/stubs. Specifically, update calls to
UrlService.create (e.g., in tests/integration/test_url_lifecycle.py and the
other occurrence around the second mention) to assign both values (doc,
raw_token) or ignore the token with underscore if unused, and modify any mock
implementations or factories to return (doc, raw_token) instead of just doc so
unpacking does not fail.
---
Nitpick comments:
In `@schemas/models/url.py`:
- Line 52: UrlV2Doc currently exposes the DB-only manage_token field by default;
mark it non-serializable so model_dump() and other default serializers won't
emit it. Update the manage_token declaration on UrlV2Doc to use a Pydantic field
with exclusion (e.g., manage_token: Optional[str] = Field(None, exclude=True))
or the equivalent pydantic v2 config so the attribute is omitted from
serialization; keep the name manage_token so existing code that accesses the
value internally continues to work. Ensure you import Field from pydantic and
run tests to confirm places that call model_dump() (e.g., url service code paths
that serialize UrlV2Doc) no longer include the token.
In `@tests/integration/api_v1/test_claim.py`:
- Around line 53-68: Both test_claim_wrong_token and test_claim_already_claimed
duplicate the same logic (mocking AsyncMock.claim_url to return False and
asserting a 403); replace them with a single parametrized test that calls the
same request with different inputs. Use pytest.mark.parametrize to pass alias
and manage_token values (e.g., ("testme","wrong_token") and
("claimed","old_token")) into one test function (e.g.,
test_claim_fails_returns_403) while keeping the same setup: create user via
_make_user(), mock_svc with claim_url = AsyncMock(return_value=False), build the
app via _build_test_app({get_current_user: ..., get_url_service: ...}), perform
the POST to "/api/v1/claim", and assert resp.status_code == 403.
- Line 1: The module-level docstring currently states only "Tests for POST
/api/v1/claim" but the file also contains tests for the /api/v1/shorten endpoint
(see tests around lines referencing shorten tests), so update the top docstring
to accurately reflect both endpoints (e.g., "Tests for POST /api/v1/claim and
POST /api/v1/shorten") or move the shorten-related test functions into their
dedicated shorten test file; ensure you update any import/fixture references if
relocating tests and keep function names like the shorten test functions
unchanged.
- Around line 14-80: Add two tests to TestClaim: one (e.g.,
test_claim_validation_errors) that posts to "/api/v1/claim" with missing or
empty "alias" and/or "manage_token" and asserts a 422 response (validation
error) using _build_test_app with get_current_user returning a valid user and
get_url_service a mock; and another (e.g., test_claim_service_exception) that
configures mock_svc.claim_url to raise an exception
(side_effect=Exception("boom")) and asserts the request to the claim endpoint
yields a 500 (or propagates as server error) by using TestClient(application,
raise_server_exceptions=False) so you verify the service exception path in the
existing TestClaim test class.
🪄 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: c6215685-7465-4d78-b6c0-a65c298ac939
📒 Files selected for processing (17)
repositories/url_repository.pyroutes/api_v1/__init__.pyroutes/api_v1/claim.pyroutes/api_v1/shorten.pyschemas/dto/responses/url.pyschemas/models/url.pyservices/url_service.pystatic/css/index.cssstatic/css/result.cssstatic/js/auth.jsstatic/js/index-recent-links.jsstatic/js/index-script.jsstatic/js/result-script.jstemplates/index.htmltemplates/result.htmltests/integration/api_v1/test_claim.pytests/integration/api_v1/test_shorten.py
Comment on lines
+257
to
+263
| { | ||
| "$set": { | ||
| "owner_id": new_owner_id, | ||
| "manage_token": None, | ||
| "updated_at": datetime.now(timezone.utc), | ||
| } | ||
| }, |
There was a problem hiding this comment.
Claiming should normalize private_stats when ownership changes.
Anonymous URLs are created with private_stats=None in services/url_service.py, Line 235-237, and check_stats_privacy() treats None as public. This update transfers ownership but leaves that field untouched, so claimed links stay public and violate the UrlV2Doc invariant from schemas/models/url.py, Line 48.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@repositories/url_repository.py` around lines 257 - 263, When claiming
ownership in the update that sets "owner_id" and "manage_token" (the block using
new_owner_id), also normalize the private_stats field so it is never left as
None: read the current document's private_stats and set "$set":
{"private_stats": False} if it's None (or set to the existing value if already a
boolean) so UrlV2Doc's boolean invariant is preserved; update the same $set
payload that contains owner_id/manage_token/updated_at to include the normalized
private_stats value.
Comment on lines
+45
to
+50
| """ | ||
| Transfer ownership of an anonymously created URL to your account. | ||
|
|
||
| The manage_token is single-use and is invalidated immediately on success. | ||
| Returns 403 if the token is wrong, 409 if already claimed, 404 if not found. | ||
| """ |
There was a problem hiding this comment.
Docstring is inconsistent with actual implementation.
The docstring states "Returns 403 if the token is wrong, 409 if already claimed, 404 if not found" but the implementation correctly returns 403 for all failure cases (to avoid oracle attacks). Update the docstring to match the actual behavior.
📝 Proposed fix
"""
Transfer ownership of an anonymously created URL to your account.
The manage_token is single-use and is invalidated immediately on success.
- Returns 403 if the token is wrong, 409 if already claimed, 404 if not found.
+ Returns 403 if the claim fails (wrong token, already claimed, or URL not found).
"""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@routes/api_v1/claim.py` around lines 45 - 50, Update the docstring for the
claim transfer endpoint in routes/api_v1/claim.py (the function that "Transfer
ownership of an anonymously created URL to your account") to reflect actual
behavior: note that all failure cases return 403 (not distinct 403/409/404) to
avoid oracle attacks and that the manage_token is single-use and invalidated on
success; keep the short description and single-use token line but replace the
incorrect "Returns 403 if the token is wrong, 409 if already claimed, 404 if not
found" sentence with a single statement that failures return 403 for any error
condition.
- Ran 'uvx ruff format .' to format Python files - Updated error_handler.py to return 400 for validation errors - Refactored index-recent-links.js, customNotification.js, auth.js to prevent XSS - Verified all tests in tests/shorten.py pass
There was a problem hiding this comment.
🧹 Nitpick comments (1)
routes/api_v1/claim.py (1)
22-26: Consider adding length/pattern constraints to request fields.The
aliasandmanage_tokenfields accept arbitrary strings. Adding constraints would enable early rejection of obviously invalid requests before hitting the database:♻️ Suggested improvement
class ClaimUrlRequest(BaseModel): - alias: str = Field(..., description="The short alias to claim.") + alias: str = Field( + ..., + min_length=1, + max_length=100, + description="The short alias to claim.", + ) manage_token: str = Field( - ..., description="The one-time token returned at creation." + ..., + min_length=32, + max_length=64, + description="The one-time token returned at creation.", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@routes/api_v1/claim.py` around lines 22 - 26, The ClaimUrlRequest model currently accepts arbitrary strings for alias and manage_token; tighten validation by adding length and pattern constraints on alias and manage_token in ClaimUrlRequest (use Field parameters like min_length, max_length and regex) to reject invalid input early — for example enforce an alias pattern (alphanumeric/allowed chars, length limits) and a manage_token length/format (fixed length or base64/hex regex) so Pydantic will validate before DB access.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@routes/api_v1/claim.py`:
- Around line 22-26: The ClaimUrlRequest model currently accepts arbitrary
strings for alias and manage_token; tighten validation by adding length and
pattern constraints on alias and manage_token in ClaimUrlRequest (use Field
parameters like min_length, max_length and regex) to reject invalid input early
— for example enforce an alias pattern (alphanumeric/allowed chars, length
limits) and a manage_token length/format (fixed length or base64/hex regex) so
Pydantic will validate before DB access.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e315bcf-690e-4988-bbbd-14d564fab985
📒 Files selected for processing (9)
middleware/error_handler.pyrepositories/url_repository.pyroutes/api_v1/claim.pyschemas/dto/responses/url.pyschemas/models/url.pyservices/url_service.pystatic/js/auth.jsstatic/js/customNotification.jsstatic/js/index-recent-links.js
✅ Files skipped from review due to trivial changes (1)
- middleware/error_handler.py
🚧 Files skipped from review as they are similar to previous changes (4)
- schemas/models/url.py
- schemas/dto/responses/url.py
- static/js/index-recent-links.js
- static/js/auth.js
…ple, update smoke test route count
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Dockerfile (1)
1-4: Pin container images to immutable digests for reproducibility and supply-chain security.Both
python:3.12-slimandghcr.io/astral-sh/uv:latestuse mutable tags. Instead, pin images to SHA256 digests using the@sha256:...syntax:FROM python:3.12@sha256:<digest> COPY --from=ghcr.io/astral-sh/uv@sha256:<digest> /uv /uvx /bin/To find the current digest for each image, run
docker manifest inspect <image>:<tag>or check the GitHub container registry. This ensures reproducible builds and prevents unexpected image mutations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 1 - 4, The Dockerfile uses mutable tags in the FROM and COPY --from lines (FROM python:3.12-slim and COPY --from=ghcr.io/astral-sh/uv:latest); replace both image references with their immutable digest forms using the `@sha256`:<digest> syntax (e.g., FROM python@sha256:<digest> and COPY --from=ghcr.io/astral-sh/uv@sha256:<digest>) so builds are reproducible and secure; obtain the correct digests with docker manifest inspect or the registry UI and substitute them into the FROM and COPY --from image references.tests/shorten.py (2)
122-132: Consider asserting the specific expected status code for each validation case.Accepting both
400and422in these assertions reduces test precision:
- Line 132 (missing URL): A missing required field is handled by Pydantic schema validation, which returns
422 Unprocessable Entity. This should consistently be422.- Line 122 (invalid URL): Depends on where URL format validation occurs—Pydantic validators return
422, service-layer checks return400.If the API behavior is intentionally flexible, consider adding a comment explaining why both codes are acceptable. Otherwise, determine the canonical validation layer and assert the specific expected code to catch regressions.
Suggested fix if validation is standardized on Pydantic
def test_invalid_url(): """Test invalid URL is rejected""" response = requests.post( f"{BASE_URL}/api/v1/shorten", json={"url": "not-a-valid-url"}, ) - assert response.status_code in [400, 422], f"Expected 400 or 422, got {response.status_code}" + assert response.status_code == 422, f"Expected 422, got {response.status_code}" print("✅ Invalid URL rejected") def test_missing_url(): """Test missing URL is rejected""" response = requests.post( f"{BASE_URL}/api/v1/shorten", json={}, ) - assert response.status_code in [400, 422], f"Expected 400 or 422, got {response.status_code}" + assert response.status_code == 422, f"Expected 422, got {response.status_code}" print("✅ Missing URL rejected")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/shorten.py` around lines 122 - 132, The tests currently accept either 400 or 422 which reduces precision; update the assertions in test_missing_url (function test_missing_url) to assert a specific status code of 422 (Pydantic validation) and update the earlier invalid-URL assertion (the test that prints "✅ Invalid URL rejected") to assert the canonical expected status code (most likely 422 if URL format is validated by Pydantic or 400 if validated in service layer) so each test checks one exact response; if the API intentionally allows both codes, instead add a clarifying comment above those tests explaining that both 400 and 422 are acceptable and why.
36-36: Duplicate alias should return a consistent, specific status code.A duplicate alias is a business logic conflict, not a validation error. This should deterministically return either
400(Bad Request) or409(Conflict), not vary between runs. Accepting both400and422here reduces test precision and could mask unintended API behavior changes.If the API currently returns inconsistent codes, consider fixing the API to return a consistent code (e.g.,
409 Conflictfor duplicate resources), then update this test to assert that specific code.Minor: Use a tuple
(400, 422)instead of a list for consistency with other status checks in this file (lines 43, 67, 109).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/shorten.py` at line 36, The test currently accepts either 400 or 422 for a duplicate alias which masks API inconsistency; change the test to assert a single, deterministic status code (preferably 409 Conflict) by replacing the current assertion on response.status_code with a strict equality check (e.g., response.status_code == 409) and, if the API is inconsistent, update the duplicate-alias handling in the service code to return 409 for duplicate resources; also replace the current list literal [400, 422] with a tuple for consistency with other status checks in this file and ensure you update any related assertions in tests/shorten.py that reference response.status_code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Around line 1-13: Add a non-root user and switch to it in the Dockerfile:
create a dedicated unprivileged user (e.g., "appuser") and group, chown the
application directory (/app) to that user after copying files and before running
"uv sync", and add a USER instruction so subsequent commands (including RUN uv
sync and CMD ["uv", ...]) run as the non-root user; ensure any required home or
tmp dirs are owned by that user and retain necessary execute permissions for
/uvx or other runtime binaries if needed.
---
Nitpick comments:
In `@Dockerfile`:
- Around line 1-4: The Dockerfile uses mutable tags in the FROM and COPY --from
lines (FROM python:3.12-slim and COPY --from=ghcr.io/astral-sh/uv:latest);
replace both image references with their immutable digest forms using the
`@sha256`:<digest> syntax (e.g., FROM python@sha256:<digest> and COPY
--from=ghcr.io/astral-sh/uv@sha256:<digest>) so builds are reproducible and
secure; obtain the correct digests with docker manifest inspect or the registry
UI and substitute them into the FROM and COPY --from image references.
In `@tests/shorten.py`:
- Around line 122-132: The tests currently accept either 400 or 422 which
reduces precision; update the assertions in test_missing_url (function
test_missing_url) to assert a specific status code of 422 (Pydantic validation)
and update the earlier invalid-URL assertion (the test that prints "✅ Invalid
URL rejected") to assert the canonical expected status code (most likely 422 if
URL format is validated by Pydantic or 400 if validated in service layer) so
each test checks one exact response; if the API intentionally allows both codes,
instead add a clarifying comment above those tests explaining that both 400 and
422 are acceptable and why.
- Line 36: The test currently accepts either 400 or 422 for a duplicate alias
which masks API inconsistency; change the test to assert a single, deterministic
status code (preferably 409 Conflict) by replacing the current assertion on
response.status_code with a strict equality check (e.g., response.status_code ==
409) and, if the API is inconsistent, update the duplicate-alias handling in the
service code to return 409 for duplicate resources; also replace the current
list literal [400, 422] with a tuple for consistency with other status checks in
this file and ensure you update any related assertions in tests/shorten.py that
reference response.status_code.
🪄 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: 22189dad-c50a-47a5-8739-5aaa7cef5787
📒 Files selected for processing (5)
Dockerfiletests/integration/test_url_lifecycle.pytests/shorten.pytests/smoke/test_routes_registered.pytests/unit/services/test_url_service.py
…atus codes, stabilize V2 creation, pass integration tests
There was a problem hiding this comment.
♻️ Duplicate comments (1)
repositories/url_repository.py (1)
257-263:⚠️ Potential issue | 🟠 MajorNormalize
private_statsduring ownership transfer.Line 258-262 updates ownership but leaves
private_statsuntouched. If legacy/anonymous docs still carryprivate_stats=None, claim can preserve an invalid/non-normalized state after transfer.Suggested atomic fix (preserve bool, coerce null→false)
- result = await self._col.update_one( + result = await self._col.update_one( { "alias": alias, "owner_id": ANONYMOUS_OWNER_ID, "manage_token": token_hash, }, - { - "$set": { - "owner_id": new_owner_id, - "manage_token": None, - "updated_at": datetime.now(timezone.utc), - } - }, + [ + { + "$set": { + "owner_id": new_owner_id, + "manage_token": None, + "updated_at": datetime.now(timezone.utc), + "private_stats": {"$ifNull": ["$private_stats", False]}, + } + } + ], )To verify this is still needed:
#!/bin/bash set -euo pipefail # 1) Confirm Url model invariant/type for private_stats fd -i 'url.py' | xargs -r rg -n -C2 '\bprivate_stats\b' # 2) Confirm how anonymous docs initialize private_stats in create flow fd -i 'url_service.py' | xargs -r rg -n -C4 'private_stats|ANONYMOUS_OWNER_ID|manage_token|doc\["private_stats"\]' # 3) Confirm whether repo currently normalizes private_stats on claim rg -n -C3 'claim_by_manage_token|private_stats|update_one\(' repositories/url_repository.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@repositories/url_repository.py` around lines 257 - 263, The ownership-transfer update in claim_by_manage_token currently sets owner_id/manage_token/updated_at but leaves private_stats possibly None; change the update to an aggregation-style update that atomically normalizes private_stats by setting it to the existing value if boolean or coercing null->False (e.g., use a $set with an expression like $ifNull on "$private_stats" to default to false) while still setting owner_id=new_owner_id, manage_token=None and updated_at=datetime.now(timezone.utc) so the operation remains atomic and preserves existing boolean values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@repositories/url_repository.py`:
- Around line 257-263: The ownership-transfer update in claim_by_manage_token
currently sets owner_id/manage_token/updated_at but leaves private_stats
possibly None; change the update to an aggregation-style update that atomically
normalizes private_stats by setting it to the existing value if boolean or
coercing null->False (e.g., use a $set with an expression like $ifNull on
"$private_stats" to default to false) while still setting owner_id=new_owner_id,
manage_token=None and updated_at=datetime.now(timezone.utc) so the operation
remains atomic and preserves existing boolean values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8e52e8c-ed3f-48b0-ae32-ac7f5ced09db
📒 Files selected for processing (8)
repositories/url_repository.pyroutes/api_v1/shorten.pyschemas/dto/responses/url.pyschemas/models/url.pyservices/url_service.pytests/integration/api_v1/test_shorten.pytests/shorten.pytests/unit/services/test_url_service.py
✅ Files skipped from review due to trivial changes (2)
- tests/unit/services/test_url_service.py
- services/url_service.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/integration/api_v1/test_shorten.py
- tests/shorten.py
- schemas/models/url.py
Member
|
@revanthlol will come back to this soon! |
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.
What does this PR do?
Fixes the UX issue where anonymous users couldn't manage URLs they created
before signing up. Closes #
How it works
When an anonymous user shortens a URL, the backend generates a cryptographically
random
manage_token, stores its SHA-256 hash in the URL document, and returnsthe raw token once in the API response.
The frontend stores the token inside the existing
recentURLslocalStoragestructure as
{ alias, manage_token }.On the result page, a banner shows the token with a copy button and a
"Claim this link" option that opens the auth modal.
On the homepage, unclaimed URLs show an Unclaimed badge and a
"Sign in to claim all your links" button.
After successful login or registration,
sweepAndClaimTokens()automaticallyclaims all stored tokens in one pass before redirecting.
The
/api/v1/claimendpoint is atomic — a singleupdate_onewith allconditions in the filter (owner is anonymous + token hash matches). Token is
nulled immediately after claim, making replay impossible.
Security
manage_tokenis excluded from all other API responsesFiles changed
Backend
schemas/models/url.py— addedmanage_token: Optional[str]toUrlV2Docrepositories/url_repository.py— addedclaim_by_manage_token()services/url_service.py— token generation increate(), newclaim_url()routes/api_v1/claim.py— newPOST /api/v1/claimendpointroutes/api_v1/shorten.py— unpack tuple return, expose token in responseschemas/dto/responses/url.py— addedmanage_tokenfield toUrlResponseFrontend
static/js/index-script.js— store token inrecentURLson shortenstatic/js/index-recent-links.js— Unclaimed badge + Claim All buttonstatic/js/result-script.js— token banner, claimNow() → auth modal flowstatic/js/auth.js—sweepAndClaimTokens()on login/register successstatic/css/result.css— token banner stylesstatic/css/index.css— badge + claim button stylesTests
tests/integration/api_v1/test_claim.py— 6 new integration teststests/integration/api_v1/test_shorten.py— updated mocks for tuple returncloses issue anonymous users can't reclaim their links #133
Summary by Sourcery
Add support for anonymous URL ownership claims using one-time manage tokens surfaced in shorten responses and claimable after authentication.
New Features:
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Style
Tests
Chores