fix: scope the self-link check to the destination host - #287
Conversation
Raise the click DLQ ceiling via CLICK_EVENTS_DLQ_MAXLEN (default 100k, hours of clicks instead of minutes) and add scripts/replay_dlq.py to feed dead-lettered events back through the stream after an outage.
Explicit nulls flip a bucket field's BSON type and force the bucket to close early, fragmenting the collection into a few measurements per bucket. Absent fields pack fine and queries are unaffected: MongoDB treats missing like null in $eq/$group and the stats sentinels already map both.
Sparse per-link traffic degenerates 1-hour bucket spans into one bucket per click. New deploys get hours (30-day spans); existing collections keep their setting and need a one-time collMod, since granularity can only be coarsened.
validate_url rejected any URL containing "spoo.me" anywhere, so a destination that merely mentioned it in a path or query was refused. Match on the parsed host (or a subdomain of it) instead, which is the only case that actually loops back to us.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds null-excluding click serialization, hourly time-series granularity, hostname-scoped URL validation, configurable DLQ capacity, and a Redis DLQ replay utility with tests. ChangesClick data and DLQ handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: 🟡 Moderate · up to The URL validation fix is localized, but existing click collections may retain their previous data granularity because the required startup upgrade is not applied. Merge should wait for a fix or explicit owner acceptance of this bounded correctness risk. Sequence Diagram(s)sequenceDiagram
participant Operator
participant replay_dlq.py
participant RedisDLQ
participant RedisMainStream
Operator->>replay_dlq.py: Run replay command
replay_dlq.py->>RedisDLQ: Read DLQ entries in batches
replay_dlq.py->>RedisMainStream: Add valid payloads
replay_dlq.py->>RedisDLQ: Delete replayed entries when enabled
replay_dlq.py-->>Operator: Report totals and group statistics
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/unit/services/test_click_service.py (1)
306-310: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the test to match the persisted contract.
test_utm_tags_default_to_nonenow asserts omission from the document, not a storedNonevalue. Rename the test so future failures do not suggest the wrong behavior.🤖 Prompt for 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. In `@tests/unit/services/test_click_service.py` around lines 306 - 310, Rename the test function test_utm_tags_default_to_none to reflect that absent UTM tags are omitted from the persisted document rather than stored as None. Keep the existing assertions and test behavior unchanged.
🤖 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 `@repositories/indexes.py`:
- Around line 67-71: Update the existing clicks-collection setup around
create_collection to catch the already-exists case and run a dedicated,
idempotent collMod migration that sets time-series granularity to "hours".
Preserve normal creation behavior for new collections, avoid masking unexpected
errors, and add coverage for the existing-collection migration path.
In `@scripts/replay_dlq.py`:
- Around line 76-79: Update the replay loop in scripts/replay_dlq.py to validate
each __data__ payload with the same ClickEvent.model_validate_json contract used
by services/click/events.py before XADD. Treat validation failures like missing
data: increment skipped, continue without replaying, and leave the invalid entry
in the DLQ; only validated events should be sent to consumer groups and counted
as successful.
Apply the same fix in `@scripts/replay_dlq.py` at line 88.
Apply the same fix in `@scripts/replay_dlq.py` around lines 88 - 90.
In `@shared/validators.py`:
- Around line 53-57: Update validate_url around urlparse to catch ValueError and
return False for malformed inputs such as "https://[" before applying the scheme
allowlist check. Add a regression test covering that input and asserting
validation returns False.
---
Nitpick comments:
In `@tests/unit/services/test_click_service.py`:
- Around line 306-310: Rename the test function test_utm_tags_default_to_none to
reflect that absent UTM tags are omitted from the persisted document rather than
stored as None. Keep the existing assertions and test behavior unchanged.
🪄 Autofix
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: dc8199d0-ad07-4c1f-8cd3-e116ba59f6e3
📒 Files selected for processing (12)
config.pyrepositories/indexes.pyschemas/models/click.pyscripts/replay_dlq.pyshared/validators.pytests/unit/repositories/test_indexes.pytests/unit/schemas/models/test_click.pytests/unit/services/test_click_service.pytests/unit/shared/test_validators.pytests/unit/workers/test_dlq_guard.pyworkers/click_worker.pyworkers/dlq.py
| # "hours" spans buckets over 30 days. Most links get sparse | ||
| # clicks, so finer granularity degenerates into one bucket | ||
| # per click. Existing deploys need a one-time collMod to | ||
| # match (the transition is one-way, coarser only). | ||
| "granularity": "hours", |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file ---'
sed -n '1,130p' repositories/indexes.py
printf '%s\n' '--- related references ---'
rg -n -S 'create_collection|collMod|granularity|clicks|timeseries' . \
-g '!node_modules' -g '!dist' -g '!build' | head -250
printf '%s\n' '--- repository metadata ---'
git status --short
git diff --statRepository: spoo-me/spoo
Length of output: 30656
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- index tests ---'
sed -n '1,230p' tests/unit/repositories/test_indexes.py
printf '%s\n' '--- bootstrap and migration files ---'
git ls-files infrastructure/bootstrap migrations 2>/dev/null || true
find infrastructure -maxdepth 3 -type f -print 2>/dev/null | sort
printf '%s\n' '--- migration/lifespan references ---'
rg -n -S 'bootstrap|migration|ensure_indexes|lifespan|MongoDB|MONGO|mongo' app.py infrastructure docker-compose*.yml README.md pyproject.toml requirements*.txt 2>/dev/null | head -300Repository: spoo-me/spoo
Length of output: 12290
🌐 Web query:
MongoDB time series granularity collMod increase granularity supported versions existing collection createCollection behavior
💡 Result:
In MongoDB, you can modify the granularity of an existing time series collection using the collMod command, but you are restricted to only increasing the granularity [1][2]. Specifically, you can change the granularity from a shorter time unit to a longer one (e.g., from "seconds" to "minutes" or "hours") [1][3]. You cannot decrease the granularity [1][4]. Supported Versions and Requirements: - Changing granularity is supported in MongoDB 5.0.1 and later [3][4]. - For sharded time series collections, you must be running MongoDB 6.0 or later to modify granularity [1][3]. - When performing a collMod operation on a time series collection, the modification must be in a dedicated command, separate from any other collection modifications [2]. Modifying Granularity or Custom Bucketing: - If your collection uses the standard granularity parameter, you can increase it using: db.runCommand({ collMod: "collectionName", timeseries: { granularity: "minutes" } }) [1][2] - Starting in MongoDB 6.3, you can use custom bucketing parameters (bucketMaxSpanSeconds and bucketRoundingSeconds) instead of the standard granularity [1][5]. If you are using these custom parameters, you must include both in the collMod command and set them to the same value to increase the interval [1][2]. You cannot decrease these values [2]. Collection Creation (createCollection): - When creating a collection, you can specify granularity or use the newer custom bucketing parameters [5][6]. - The granularity options are "seconds" (default), "minutes", and "hours" [5][6]. - If you opt for custom bucketing, you must provide both bucketMaxSpanSeconds and bucketRoundingSeconds [6][7]. If you set these, you should not specify the granularity parameter [5][6]. Setting these parameters to the same value provides more precise control over bucket boundaries, which can optimize performance for fixed time-interval queries [1][8].
Citations:
- 1: https://www.mongodb.com/docs/manual/core/timeseries/timeseries-granularity/
- 2: https://www.mongodb.com/docs/manual/reference/command/collmod/
- 3: https://www.mongodb.com/docs/v8.2/core/timeseries/timeseries-granularity/
- 4: https://www.mongodb.com/docs/v7.0/core/timeseries/timeseries-granularity/
- 5: https://www.mongodb.com/docs/manual/core/timeseries/timeseries-procedures/
- 6: https://www.mongodb.com/docs/manual/reference/method/db.createcollection/
- 7: https://www.mongodb.com/docs/manual/reference/command/create/
- 8: https://www.mongodb.com/docs/v8.0/core/timeseries/timeseries-granularity/
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- startup ordering ---'
sed -n '70,165p' app.py
printf '%s\n' '--- dependency/version declarations ---'
sed -n '1,90p' requirements.txt
sed -n '1,55p' pyproject.toml
printf '%s\n' '--- all tests for index setup ---'
rg -n -C 8 'test_ensure_indexes|create_collection|CollectionInvalid|OperationFailure' tests/unit/repositories/test_indexes.py
printf '%s\n' '--- bootstrap implementation ---'
cat -n infrastructure/bootstrap/__init__.py infrastructure/bootstrap/system_default_domain.pyRepository: spoo-me/spoo
Length of output: 13314
Apply granularity: "hours" to existing clicks collections.
When clicks already exists, create_collection() raises an expected error and the code skips the change. Add a dedicated, idempotent collMod migration and test this path. MongoDB supports increasing time-series granularity with collMod on supported server versions.
🤖 Prompt for 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.
In `@repositories/indexes.py` around lines 67 - 71, Update the existing
clicks-collection setup around create_collection to catch the already-exists
case and run a dedicated, idempotent collMod migration that sets time-series
granularity to "hours". Preserve normal creation behavior for new collections,
avoid masking unexpected errors, and add coverage for the existing-collection
migration path.
| data = fields.get(STREAM_FIELD_DATA) | ||
| if data is None: | ||
| skipped += 1 | ||
| continue |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make DLQ replay preserve the click publication contract.
Before XADD, validate __data__ with the same ClickEvent contract; empty or malformed payloads must remain in the DLQ and be reported as skipped. Replay must pass the configured maxlen, approximate=True, and ref_policy="ACKED" so a large replay cannot bypass stream retention and pressure Redis into the inline fallback. When deleting entries (--keep is not set), make XADD and XDEL atomic, for example with one Lua script, so a partial failure cannot leave a replayable entry and cause duplicates.
📍 Affects 1 file
scripts/replay_dlq.py#L76-L79(this comment)scripts/replay_dlq.py#L88-L88scripts/replay_dlq.py#L88-L90
🤖 Prompt for 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.
In `@scripts/replay_dlq.py` around lines 76 - 79, Update the replay loop in
scripts/replay_dlq.py to validate each __data__ payload with the same
ClickEvent.model_validate_json contract used by services/click/events.py before
XADD. Treat validation failures like missing data: increment skipped, continue
without replaying, and leave the invalid entry in the DLQ; only validated events
should be sent to consumer groups and counted as successful.
Apply the same fix in `@scripts/replay_dlq.py` at line 88.
Apply the same fix in `@scripts/replay_dlq.py` around lines 88 - 90.
Zingzy
left a comment
Zingzy
left a comment
There was a problem hiding this comment.
Reviewed the full diff and ran the five affected test files in a clean worktree: 95 passed. Mergeable, nothing blocking; inline notes below.
The host-scoped check is the right semantic: the host alone decides whether a request loops back, and urlparse().hostname already strips userinfo and the port, so both smuggling directions stay closed. The normalization matches system_default_domain (lowercase, strip trailing dots), so the derived blocklist compares cleanly. The to_mongo override preserves the base contract too: exclude_none=True drops a None _id the same way the base's explicit pop does.
Deploy ordering: this goes out before spoo-me/frontend#42, otherwise the form accepts URLs the API still rejects. Merging ships nothing either way.
One trade worth stating out loud: the old substring check accidentally rejected impersonation URLs like https://phisher.example/spoo.me/login, and those now validate. Loop prevention was never the right place to catch them; if they should be caught, the regex blocklist is the honest home.
| # Scheme allowlist defends against ftp/file/data/etc even if the | ||
| # validators package widens its accepted schemes upstream. | ||
| if urlparse(url).scheme not in _ALLOWED_URL_SCHEMES: | ||
| parsed = urlparse(url) |
There was a problem hiding this comment.
urlparse itself raises on a malformed IPv6 authority: urlparse("https://[::1") throws ValueError: Invalid IPv6 URL. long_url is a plain str in the DTO, so the raw string reaches this line and the exception escapes as a 500 on the shorten endpoints. Pre-existing (the old code called urlparse(url).scheme too), but since this PR touches the exact line: wrap the parse in try/except ValueError: return False and add https://[::1 to the parametrized cases. Anonymous rate-limited surface, so the cost today is Sentry noise and an ugly 500 instead of a clean 400.
| """Replay dead-lettered click events back onto the main stream. | ||
|
|
||
| Standalone — reads ``CLICK_EVENTS_QUEUE_REDIS_URI`` from the environment. | ||
| Replayed events fan out to every consumer group again (streams have no |
There was a problem hiding this comment.
The fan-out note stops one sentence short of the consequence. A message dead-lettered by one group was already processed and acked by the others, so replaying re-delivers to every group: duplicate hotness increments, duplicate click inserts when stats had succeeded (no dedupe key on a time-series insert), and duplicate webhook fires under a fresh stream id, which consumer-side idempotency will not catch. After a global outage the replay is clean; after a partial-group failure it is not, and the per-group breakdown this script prints is the signal to read first. Worth saying that here so the operator runs --dry-run, checks the group counts, and decides with eyes open.
| group: str, | ||
| dlq_stream: str, | ||
| max_deliveries: int, | ||
| dlq_maxlen: int = DLQ_MAXLEN, |
There was a problem hiding this comment.
Two bounds exist now: this constant (10k, still the constructor default) and the config default (100k). Prod always takes the config path, so the default here is a fallback nobody uses, and the comment above DLQ_MAXLEN tells a different sizing story than the one in config.py. Drop the parameter default and let config be the single source; the tests already pass it explicitly.
| # clicks, so finer granularity degenerates into one bucket | ||
| # per click. Existing deploys need a one-time collMod to | ||
| # match (the transition is one-way, coarser only). | ||
| "granularity": "hours", |
There was a problem hiding this comment.
The collMod note is right but nothing operational carries it; add the one-time collMod to the release checklist so it does not evaporate between merge and release. Also worth remembering that this and the exclude_none change only help buckets written afterwards. The existing fragmented backlog stays as it is unless the collection is rewritten; letting it age is probably fine, but that is a decision, not a default.
urlparse raises ValueError on "https://[::1", and long_url reaches the validator as a plain str, so the exception escaped as a 500 on the shorten endpoints rather than a clean rejection.
validate_urltreated any URL containing the stringspoo.meas self-referential, including matches in the path, query or fragment. Shortening an analytics dashboard URL filtered onspoo.mewas rejected with "URL is not allowed or invalid", and the dashboard surfaced it as "Short links can't point back at spoo.me."A destination only loops back to us when the request would actually arrive here, which the host decides on its own. This matches on the parsed host instead, or a subdomain of it.
urlparse().hostnamedrops userinfo and the port, sohttps://example.com@spoo.me/is still blocked andhttps://spoo.me@example.com/is not.The frontend mirrors this validator to pre-validate the field, so spoo-me/frontend#42 carries the matching change. This one needs to deploy first, otherwise the form accepts a URL the API still rejects.
Testing
Existing cases still pass, including the
www.spoo.meone that now relies on the subdomain clause. Added coverage for path, query and fragment mentions, the lookalike host, and both userinfo directions. 58 passed.Summary by CodeRabbit