Feat(#7): 단축 URL 리다이렉트 추적 및 원본 URL 복원 로직 구현 - #9
Conversation
📝 WalkthroughWalkthroughAdds URL extraction and asynchronous short-URL resolution utilities. URL extraction trims punctuation, preserves order, and deduplicates results. Redirect resolution manually follows redirects with HEAD/GET handling, limits, timeouts, logging, and exception recovery, with corresponding tests. ChangesURL service utilities
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant resolve_short_url
participant HTTP endpoint
Caller->>resolve_short_url: URL and resolution limits
resolve_short_url->>HTTP endpoint: HEAD request
HTTP endpoint-->>resolve_short_url: Redirect and Location header
resolve_short_url->>HTTP endpoint: HEAD request for next URL
HTTP endpoint-->>resolve_short_url: Final response
resolve_short_url-->>Caller: Resolved or last URL
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 8
🧹 Nitpick comments (2)
app/service/url/tracer.py (2)
7-7: 🚀 Performance & Scalability | 🔵 TrivialUse a total resolution deadline.
timeoutapplies independently to every hop; the defaults can spend roughly 25 seconds across five redirects. If this runs on a request path, use an absolute deadline and pass the remaining budget to each request.Also applies to: 13-16
🤖 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 `@app/service/url/tracer.py` at line 7, Update resolve_short_url so timeout represents one total resolution budget rather than a per-redirect timeout: establish an absolute deadline before following redirects, compute the remaining time before each request, and pass that remaining budget to the request. Stop or fail resolution when the deadline is exhausted while preserving the max_redirects limit.
36-40: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNarrow the exception handling.
HTTPX raises
HTTPStatusErrorwhenraise_for_status()is called, which this code never does; request failures are represented byRequestErrorsubclasses. CatchingExceptionmasks programming errors and silently returns partial results. Catch explicit request/URL exceptions instead, and use{e!s}if retaining explicit conversion. (python-httpx.org)🤖 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 `@app/service/url/tracer.py` around lines 36 - 40, Update the exception handling in the URL tracing flow to catch HTTPX request failures via RequestError and explicit URL-related exceptions instead of HTTPStatusError and broad Exception. Preserve the existing logging and loop behavior, using {e!s} for explicit exception string conversion if needed, while allowing unexpected programming errors to propagate.Sources: MCP tools, Linters/SAST tools
🤖 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/service/url/extractor.py`:
- Around line 3-4: Update the URL_PATTERN compilation to use re.IGNORECASE so
HTTP and HTTPS schemes are matched regardless of casing, while preserving the
existing URL character pattern and redirect-analysis flow.
- Around line 17-22: Update the URL cleanup loop in the extractor to remove
trailing punctuation only when it is clearly sentence-delimiting, preserving
valid URL characters such as the closing bracket in https://[::1] and
path-ending ! or ). Add regression tests covering these cases and ordinary
sentence punctuation cleanup.
In `@app/service/url/tracer.py`:
- Line 28: Update the redirect logging around the logger.info calls in the URL
tracing flow to avoid emitting full user-controlled URLs. Sanitize current_url
before logging by retaining only a safe origin/path, or use an equivalent hash
or request ID, and apply the same protection to all related redirect log
messages.
- Around line 22-28: Update the redirect handling around current_url so Location
values are resolved against the current URL, producing an absolute target for
relative redirects. Validate the resolved target’s scheme and only update
current_url for http or https URLs; otherwise stop processing, while preserving
the existing missing-Location behavior and logging.
- Around line 15-20: Broaden the fallback logic around the client.head call in
the URL tracing flow to retry with GET for the supported HEAD-rejection
statuses, including 403 and 501, and make the status collection explicit and
reusable. Add deterministic tests covering each supported fallback status and
confirming GET is not used for other statuses.
- Around line 9-16: Update the URL resolution flow around current_url and the
redirect loop to validate the initial URL before the first HEAD request and
validate every Location target before following it. Reuse or add SSRF validation
covering allowed schemes, hostnames, and resolved IP addresses, rejecting
localhost, private, link-local, and metadata endpoints before any outbound
request.
In `@tests/url/test_url_tracer.py`:
- Around line 29-34: Update test_max_redirect_limit to use a positive
max_redirects value and mock a redirect chain that exceeds that limit. Assert
that resolve_short_url stops after the configured number of requests and returns
the URL reached at exhaustion, verifying both redirect-limit enforcement and
request count.
- Around line 6-26: Replace the live-network calls in
test_normal_url_no_redirect, test_tinyurl_resolution, and
test_broken_url_graceful_handling with a deterministic HTTPX mock transport or
local test server. Configure the transport to return the required redirect,
final-response, and failure scenarios, then assert exact final URLs and preserve
the fallback-to-original-URL behavior without relying on Google, TinyURL, DNS,
or external network access.
---
Nitpick comments:
In `@app/service/url/tracer.py`:
- Line 7: Update resolve_short_url so timeout represents one total resolution
budget rather than a per-redirect timeout: establish an absolute deadline before
following redirects, compute the remaining time before each request, and pass
that remaining budget to the request. Stop or fail resolution when the deadline
is exhausted while preserving the max_redirects limit.
- Around line 36-40: Update the exception handling in the URL tracing flow to
catch HTTPX request failures via RequestError and explicit URL-related
exceptions instead of HTTPStatusError and broad Exception. Preserve the existing
logging and loop behavior, using {e!s} for explicit exception string conversion
if needed, while allowing unexpected programming errors to propagate.
🪄 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: 07fe5842-c765-4a67-a738-3eeee6c108aa
📒 Files selected for processing (4)
app/service/url/extractor.pyapp/service/url/tracer.pytests/url/test_url_extractor.pytests/url/test_url_tracer.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 8
🧹 Nitpick comments (2)
app/service/url/tracer.py (2)
7-7: 🚀 Performance & Scalability | 🔵 TrivialUse a total resolution deadline.
timeoutapplies independently to every hop; the defaults can spend roughly 25 seconds across five redirects. If this runs on a request path, use an absolute deadline and pass the remaining budget to each request.Also applies to: 13-16
🤖 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 `@app/service/url/tracer.py` at line 7, Update resolve_short_url so timeout represents one total resolution budget rather than a per-redirect timeout: establish an absolute deadline before following redirects, compute the remaining time before each request, and pass that remaining budget to the request. Stop or fail resolution when the deadline is exhausted while preserving the max_redirects limit.
36-40: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNarrow the exception handling.
HTTPX raises
HTTPStatusErrorwhenraise_for_status()is called, which this code never does; request failures are represented byRequestErrorsubclasses. CatchingExceptionmasks programming errors and silently returns partial results. Catch explicit request/URL exceptions instead, and use{e!s}if retaining explicit conversion. (python-httpx.org)🤖 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 `@app/service/url/tracer.py` around lines 36 - 40, Update the exception handling in the URL tracing flow to catch HTTPX request failures via RequestError and explicit URL-related exceptions instead of HTTPStatusError and broad Exception. Preserve the existing logging and loop behavior, using {e!s} for explicit exception string conversion if needed, while allowing unexpected programming errors to propagate.Sources: MCP tools, Linters/SAST tools
🤖 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/service/url/extractor.py`:
- Around line 3-4: Update the URL_PATTERN compilation to use re.IGNORECASE so
HTTP and HTTPS schemes are matched regardless of casing, while preserving the
existing URL character pattern and redirect-analysis flow.
- Around line 17-22: Update the URL cleanup loop in the extractor to remove
trailing punctuation only when it is clearly sentence-delimiting, preserving
valid URL characters such as the closing bracket in https://[::1] and
path-ending ! or ). Add regression tests covering these cases and ordinary
sentence punctuation cleanup.
In `@app/service/url/tracer.py`:
- Line 28: Update the redirect logging around the logger.info calls in the URL
tracing flow to avoid emitting full user-controlled URLs. Sanitize current_url
before logging by retaining only a safe origin/path, or use an equivalent hash
or request ID, and apply the same protection to all related redirect log
messages.
- Around line 22-28: Update the redirect handling around current_url so Location
values are resolved against the current URL, producing an absolute target for
relative redirects. Validate the resolved target’s scheme and only update
current_url for http or https URLs; otherwise stop processing, while preserving
the existing missing-Location behavior and logging.
- Around line 15-20: Broaden the fallback logic around the client.head call in
the URL tracing flow to retry with GET for the supported HEAD-rejection
statuses, including 403 and 501, and make the status collection explicit and
reusable. Add deterministic tests covering each supported fallback status and
confirming GET is not used for other statuses.
- Around line 9-16: Update the URL resolution flow around current_url and the
redirect loop to validate the initial URL before the first HEAD request and
validate every Location target before following it. Reuse or add SSRF validation
covering allowed schemes, hostnames, and resolved IP addresses, rejecting
localhost, private, link-local, and metadata endpoints before any outbound
request.
In `@tests/url/test_url_tracer.py`:
- Around line 29-34: Update test_max_redirect_limit to use a positive
max_redirects value and mock a redirect chain that exceeds that limit. Assert
that resolve_short_url stops after the configured number of requests and returns
the URL reached at exhaustion, verifying both redirect-limit enforcement and
request count.
- Around line 6-26: Replace the live-network calls in
test_normal_url_no_redirect, test_tinyurl_resolution, and
test_broken_url_graceful_handling with a deterministic HTTPX mock transport or
local test server. Configure the transport to return the required redirect,
final-response, and failure scenarios, then assert exact final URLs and preserve
the fallback-to-original-URL behavior without relying on Google, TinyURL, DNS,
or external network access.
---
Nitpick comments:
In `@app/service/url/tracer.py`:
- Line 7: Update resolve_short_url so timeout represents one total resolution
budget rather than a per-redirect timeout: establish an absolute deadline before
following redirects, compute the remaining time before each request, and pass
that remaining budget to the request. Stop or fail resolution when the deadline
is exhausted while preserving the max_redirects limit.
- Around line 36-40: Update the exception handling in the URL tracing flow to
catch HTTPX request failures via RequestError and explicit URL-related
exceptions instead of HTTPStatusError and broad Exception. Preserve the existing
logging and loop behavior, using {e!s} for explicit exception string conversion
if needed, while allowing unexpected programming errors to propagate.
🪄 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: 07fe5842-c765-4a67-a738-3eeee6c108aa
📒 Files selected for processing (4)
app/service/url/extractor.pyapp/service/url/tracer.pytests/url/test_url_extractor.pytests/url/test_url_tracer.py
🛑 Comments failed to post (8)
app/service/url/extractor.py (2)
3-4: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Match HTTP schemes case-insensitively.
Valid URI schemes are case-insensitive, but this regex misses
HTTP://andHTTPS://; those URLs are silently omitted and never reach redirect analysis. Addre.IGNORECASEand cover uppercase input. (rfc-editor.org)🤖 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 `@app/service/url/extractor.py` around lines 3 - 4, Update the URL_PATTERN compilation to use re.IGNORECASE so HTTP and HTTPS schemes are matched regardless of casing, while preserving the existing URL character pattern and redirect-analysis flow.Source: MCP tools
17-22: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid destructive terminal punctuation stripping.
rstrip()removes characters even when they are part of a valid URL, such as the closing bracket inhttps://[::1]or a path ending in!/). Use delimiter-aware cleanup and add regression tests.🤖 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 `@app/service/url/extractor.py` around lines 17 - 22, Update the URL cleanup loop in the extractor to remove trailing punctuation only when it is clearly sentence-delimiting, preserving valid URL characters such as the closing bracket in https://[::1] and path-ending ! or ). Add regression tests covering these cases and ordinary sentence punctuation cleanup.app/service/url/tracer.py (4)
9-16: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail printf '\n== file list ==\n' git ls-files app/service/url/tracer.py printf '\n== outline ==\n' ast-grep outline app/service/url/tracer.py --view expanded || true printf '\n== read target file ==\n' cat -n app/service/url/tracer.py | sed -n '1,220p' printf '\n== search for SSRF / URL validation related code ==\n' rg -n --hidden -S "SSRF|private network|metadata|link-local|allowlist|blocklist|resolved IP|dns rebinding|url policy|scheme|netloc|urlparse|ipaddress|localhost|169\.254\.169\.254|127\.0\.0\.1" app || trueRepository: SafeFam/SafeFam_AI
Length of output: 2391
🏁 Script executed:
set -euo pipefail echo "ping"Repository: SafeFam/SafeFam_AI
Length of output: 161
Add SSRF checks before each redirect hop. This resolver requests the extracted URL and every
Locationtarget directly; enforce scheme, host, and resolved-IP allow/deny checks on the initial URL and each redirect to block localhost, private, link-local, and metadata endpoints.🤖 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 `@app/service/url/tracer.py` around lines 9 - 16, Update the URL resolution flow around current_url and the redirect loop to validate the initial URL before the first HEAD request and validate every Location target before following it. Reuse or add SSRF validation covering allowed schemes, hostnames, and resolved IP addresses, rejecting localhost, private, link-local, and metadata endpoints before any outbound request.
15-20: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -e printf '\n== tracer.py outline ==\n' ast-grep outline app/service/url/tracer.py --view expanded || true printf '\n== tracer.py lines ==\n' nl -ba app/service/url/tracer.py | sed -n '1,220p' printf '\n== search for head/get fallback references ==\n' rg -n "head\(current_url|status_code in \[400, 404, 405\]|501|403|fallback" app test tests . || trueRepository: SafeFam/SafeFam_AI
Length of output: 50375
🏁 Script executed:
set -e printf '\n== relevant files ==\n' git ls-files | rg '(^|/)(tracer\.py|.*test.*|.*spec.*)\b' || true printf '\n== tracer.py around target lines ==\n' python3 - <<'PY' from pathlib import Path p = Path('app/service/url/tracer.py') for i, line in enumerate(p.read_text().splitlines(), 1): if 1 <= i <= 120: print(f"{i:4}: {line}") PY printf '\n== focused search for URL tracing tests ==\n' rg -n "resolve_short_url|client\.head|client\.get|status_code in \[400, 404, 405\]|HEAD 요청" app . || trueRepository: SafeFam/SafeFam_AI
Length of output: 3700
🏁 Script executed:
set -e printf '\n== tests/url/test_url_tracer.py ==\n' python3 - <<'PY' from pathlib import Path p = Path('tests/url/test_url_tracer.py') for i, line in enumerate(p.read_text().splitlines(), 1): print(f"{i:4}: {line}") PY printf '\n== search for mocked status codes in tracer tests ==\n' rg -n "400|404|405|403|501|HEAD|GET|redirect|follow_redirects" tests/url/test_url_tracer.py app/service/url/tracer.py || trueRepository: SafeFam/SafeFam_AI
Length of output: 2530
Broaden the HEAD fallback policy
app/service/url/tracer.py:18-19only retriesGETfor400/404/405, so a URL that rejectsHEADwith403or501is treated as terminal even whenGETwould redirect. Make the supported fallback statuses explicit and add deterministic tests for them.🤖 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 `@app/service/url/tracer.py` around lines 15 - 20, Broaden the fallback logic around the client.head call in the URL tracing flow to retry with GET for the supported HEAD-rejection statuses, including 403 and 501, and make the status collection explicit and reusable. Add deterministic tests covering each supported fallback status and confirming GET is not used for other statuses.
22-28: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' app/service/url/tracer.pyRepository: SafeFam/SafeFam_AI
Length of output: 1780
🏁 Script executed:
python3 - <<'PY' from pathlib import Path p = Path('app/service/url/tracer.py') print(p.exists(), p) if p.exists(): for i, line in enumerate(p.read_text().splitlines(), 1): if 1 <= i <= 220: print(f"{i:4d}: {line}") PYRepository: SafeFam/SafeFam_AI
Length of output: 2082
🏁 Script executed:
python3 - <<'PY' from pathlib import Path p = Path('app/service/url/tracer.py') print('exists=', p.exists()) if p.exists(): for i, line in enumerate(p.read_text().splitlines(), 1): if i <= 220: print(f"{i:4d}: {line}") PYRepository: SafeFam/SafeFam_AI
Length of output: 2064
🏁 Script executed:
rg -n "current_url|Location|redirect" app/service/url -SRepository: SafeFam/SafeFam_AI
Length of output: 1557
🏁 Script executed:
python3 - <<'PY' from pathlib import Path print(Path('app/service/url/tracer.py').read_text()) PYRepository: SafeFam/SafeFam_AI
Length of output: 1781
Resolve relative redirect targets before reusing
current_url.
Locationcan be relative, and assigning it directly sends the next request to an invalid URL and can return a relative path instead of the final absolute URL. Join it against the current URL and only accepthttp/httpstargets before updatingcurrent_url.🤖 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 `@app/service/url/tracer.py` around lines 22 - 28, Update the redirect handling around current_url so Location values are resolved against the current URL, producing an absolute target for relative redirects. Validate the resolved target’s scheme and only update current_url for http or https URLs; otherwise stop processing, while preserving the existing missing-Location behavior and logging.Source: MCP tools
28-28: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact URLs before logging.
These log messages emit full user-controlled URLs, including possible credentials, tokens, query parameters, and PII. Log only a sanitized origin/path, a hash, or a request ID.
Also applies to: 33-40
🤖 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 `@app/service/url/tracer.py` at line 28, Update the redirect logging around the logger.info calls in the URL tracing flow to avoid emitting full user-controlled URLs. Sanitize current_url before logging by retaining only a safe origin/path, or use an equivalent hash or request ID, and apply the same protection to all related redirect log messages.tests/url/test_url_tracer.py (2)
6-26: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Replace live network assertions with deterministic tests.
These tests depend on Google, TinyURL, DNS, CI egress, and changing external redirects. They can fail unrelated to the code and do not reliably cover redirect branches. Inject an HTTPX mock transport or local test server, then assert exact final URLs. HTTPX provides transport hooks for this purpose. (python-httpx.org)
🤖 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/url/test_url_tracer.py` around lines 6 - 26, Replace the live-network calls in test_normal_url_no_redirect, test_tinyurl_resolution, and test_broken_url_graceful_handling with a deterministic HTTPX mock transport or local test server. Configure the transport to return the required redirect, final-response, and failure scenarios, then assert exact final URLs and preserve the fallback-to-original-URL behavior without relying on Google, TinyURL, DNS, or external network access.Source: MCP tools
29-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise a nonzero redirect limit.
max_redirects=0skips the loop entirely, so this test does not verify limit enforcement or the loop-exhaustion path. Mock a chain longer than the configured limit and assert the request count and returned URL.🤖 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/url/test_url_tracer.py` around lines 29 - 34, Update test_max_redirect_limit to use a positive max_redirects value and mock a redirect chain that exceeds that limit. Assert that resolve_short_url stops after the configured number of requests and returns the URL reached at exhaustion, verifying both redirect-limit enforcement and request count.
1 participant
📝 개요
악성 피싱 사이트가 위장하기 위해 주로 악용하는 단축 URL(bit.;y, tinyurl.com 등)이 유입되었을 때, HTTP 리다이렉트(301/302 Status Code)를 종착지까지 끝까지 추적하여 최종 원본 URL(Destination URL)을 알아내는 추적 엔진을 구현합니다.
🔗 관련 이슈
🎯 주요 변경 사항
1. 비동기 단축 URL 추적 엔진 구현
2. 예외 처리 및 단위 테스트 구축
tinyurl.com단축 서비스의 원본 URL 복원 검증 완료max_redirects=0) 제한이 정상 동작하는지 테스트 완료📸 사진
<
pytest를 활용한 비동기 단위테스트를 수행하여 '4개 케이스 모두 정상 통과'함을 확인>✅ PR 체크리스트
uvicorn구동 또는 테스트 코드)를 통과했습니다.Summary by CodeRabbit
New Features
Bug Fixes