Fix #106: Add URL format validation to SEO diagnostics job queue #125

Merged

typosaurus merged 1 commits from typosaurus/ticket-106 into master

2026-07-26 23:30:58 +02:00

typosaurus commented

2026-07-23 04:38:03 +02:00

Collaborator

Resolves #106.

Plan

Implementation Plan: URL Format Validation for SEO Diagnostics Job Queue

1. Problem Summary

The SeoRunForm Pydantic model (in models.py) accepts URLs with invalid schemes (e.g., "ahttps://...", /relative/path) and passes them to the background job queue. Downstream validation in guard_public_url catches these only after enqueue, wasting resources and generating failed jobs. The analogous IsslopRunForm model already applies proper URL validation — we will adapt that pattern.

2. Scope of Changes

Two files need modification:

app/core/models.py – Add URL scheme validation to the SeoRunForm.url field.
tests/api/tools/seo.py – Add test cases covering malformed, relative, and scheme-less URLs.

No other source files require changes. The existing IsslopRunForm validation functions and constants (ISSLOP_SCHEME_PATTERN, ISSLOP_URL_PATTERN) are in models.py and can be reused partially.

3. Detailed Changes

3.1 `app/core/models.py`

Current state (lines ~462–473):

class SeoRunForm(BaseModel):
    url: str = Field(..., min_length=3)
    mode: str = Field(...)
    max_pages: int = Field(..., ge=1, le=...)

Required changes:

At the top of the file, ensure the regex patterns used by IsslopRunForm are accessible (they are already defined as module-level constants). We will define a new, stricter constant for SEO-only validation:
```
SEO_URL_PATTERN = re.compile(r'^https?://[\w./:@~^-]+$')
```
(This is a subset of ISSLOP_URL_PATTERN that only accepts http:// and https://.)
Add a Pydantic @field_validator("url", mode="before") to SeoRunForm that:
- Strips whitespace.
- If there is no :// scheme indicator, prepends https://.
- If there is a scheme but it is not http or https, raises a ValueError with a clear message.
- Checks the final value against SEO_URL_PATTERN; if no match, raises a ValueError.
The logic should not import or mutate anything outside the model. It should be self-contained and reusable.

Draft code (to be placed inside SeoRunForm class):

    @field_validator("url", mode="before")
    @classmethod
    def validate_and_normalize_url(cls, value: str) -> str:
        if not value:
            raise ValueError("URL must not be empty")
        value = value.strip()
        if "://" not in value:
            value = "https://" + value
        else:
            scheme = value.split("://", 1)[0]
            if scheme not in ("http", "https"):
                raise ValueError("Only http and https URLs are allowed.")
        if not SEO_URL_PATTERN.match(value):
            raise ValueError("URL must be a valid http or https source location")
        return value

3.2 `tests/api/tools/seo.py`

Add a new test class or individual test functions. The existing tests are async; new tests should also be async and use the test client.

Test cases to add:

Test Name	Input URL	Expected HTTP Status
`test_run_rejects_malformed_scheme`	`"ahttps://example.com"`	422
`test_run_rejects_relative_path`	`"/feed"`	422 (or 200 with normalization – see note)
`test_run_accepts_missing_scheme`	`"example.com"`	200 (with normalized URL `https://example.com`)
`test_run_rejects_non_http_scheme`	`"ftp://example.com"`	422

Note on relative paths: The investigation findings note that relative paths like "/feed" currently get https:// prepended, becoming "https:///feed", which is invalid and would fail later. To maintain consistency, we should reject them as invalid (since they are not absolute URLs). The validator above will catch them because SEO_URL_PATTERN requires a hostname after the scheme. The test should therefore expect 422.

Pseudo-code for test:

@pytest.mark.asyncio
async def test_run_rejects_malformed_scheme(seo_client, seo_payload):
    payload = seo_payload.copy()
    payload["url"] = "ahttps://devplace.net/sitem"
    response = await seo_client.post("/tools/seo/run", json=payload)
    assert response.status_code == 422
    # Optionally verify error detail contains "URL" or similar

@pytest.mark.asyncio
async def test_run_rejects_relative_path(seo_client, seo_payload):
    payload = seo_payload.copy()
    payload["url"] = "/feed"
    response = await seo_client.post("/tools/seo/run", json=payload)
    assert response.status_code == 422

@pytest.mark.asyncio
async def test_run_accepts_missing_scheme(seo_client, seo_payload):
    payload = seo_payload.copy()
    payload["url"] = "example.com"
    response = await seo_client.post("/tools/seo/run", json=payload)
    assert response.status_code == 200
    data = response.json()
    # Verify the normalized URL was used in the enqueued job
    assert data.get("url") == "https://example.com"  # Adjust field name if necessary

@pytest.mark.asyncio
async def test_run_rejects_non_http_scheme(seo_client, seo_payload):
    payload = seo_payload.copy()
    payload["url"] = "ftp://example.com"
    response = await seo_client.post("/tools/seo/run", json=payload)
    assert response.status_code == 422

4. Avoiding Past Failures

Test failure: Ensure the new tests do not interfere with existing tests (use isolated fixtures if needed). The test command runs python -m pytest {test_files}; we must only run the SEO-related test file to avoid side effects. The test_command in the prompt refers to a specific test file – we must respect that.
Lint failure: After making changes, run the project's lint tool (likely ruff or flake8) and fix any new violations. The previous lint failure was from hidden .venv files being committed; this is likely a merge conflict issue, not a style issue. We must ensure we do not introduce new violations in the source code.
Merge conflict: The merge conflict appeared from rebasing onto base branch when .venv was tracked. We must ensure our branch is based on the correct base and does not include .venv files. The plan itself assumes a clean checkout.

5. Definition of Done

In app/core/models.py, add the SEO_URL_PATTERN constant and the field_validator to SeoRunForm as described.
In tests/api/tools/seo.py, add the four test functions listed above.
Run the project's test command: pip install -e '.[dev]' -q && python -m pytest tests/api/tools/seo.py – all tests pass, including the new ones and the existing five.
Run the project's lint command (e.g., ruff check app/ tests/ or similar as defined in Makefile or pyproject.toml) – no new lint violations are introduced.
Verify that no .venv or other unintended artifacts are in the branch diff.
Confirm that the validator rejects the ticket's example URL "ahttps://devplace.net/sitem" with a 422 response in the test run.

Opened automatically by Typosaurus.

Resolves #106. ## Plan ## Implementation Plan: URL Format Validation for SEO Diagnostics Job Queue ### 1. Problem Summary The `SeoRunForm` Pydantic model (in `models.py`) accepts URLs with invalid schemes (e.g., `"ahttps://..."`, `/relative/path`) and passes them to the background job queue. Downstream validation in `guard_public_url` catches these only after enqueue, wasting resources and generating failed jobs. The analogous `IsslopRunForm` model already applies proper URL validation — we will adapt that pattern. ### 2. Scope of Changes Two files need modification: - **`app/core/models.py`** – Add URL scheme validation to the `SeoRunForm.url` field. - **`tests/api/tools/seo.py`** – Add test cases covering malformed, relative, and scheme-less URLs. No other source files require changes. The existing `IsslopRunForm` validation functions and constants (`ISSLOP_SCHEME_PATTERN`, `ISSLOP_URL_PATTERN`) are in `models.py` and can be reused partially. ### 3. Detailed Changes #### 3.1 `app/core/models.py` **Current state** (lines ~462–473): ```python class SeoRunForm(BaseModel): url: str = Field(..., min_length=3) mode: str = Field(...) max_pages: int = Field(..., ge=1, le=...) ``` **Required changes**: 1. At the top of the file, ensure the regex patterns used by `IsslopRunForm` are accessible (they are already defined as module-level constants). We will define a new, stricter constant for SEO-only validation: ```python SEO_URL_PATTERN = re.compile(r'^https?://[\w./:@~^-]+$') ``` (This is a subset of `ISSLOP_URL_PATTERN` that only accepts `http://` and `https://`.) 2. Add a Pydantic `@field_validator("url", mode="before")` to `SeoRunForm` that: - Strips whitespace. - If there is no `://` scheme indicator, prepends `https://`. - If there is a scheme but it is not `http` or `https`, raises a `ValueError` with a clear message. - Checks the final value against `SEO_URL_PATTERN`; if no match, raises a `ValueError`. 3. The logic should **not** import or mutate anything outside the model. It should be self-contained and reusable. **Draft code** (to be placed inside `SeoRunForm` class): ```python @field_validator("url", mode="before") @classmethod def validate_and_normalize_url(cls, value: str) -> str: if not value: raise ValueError("URL must not be empty") value = value.strip() if "://" not in value: value = "https://" + value else: scheme = value.split("://", 1)[0] if scheme not in ("http", "https"): raise ValueError("Only http and https URLs are allowed.") if not SEO_URL_PATTERN.match(value): raise ValueError("URL must be a valid http or https source location") return value ``` #### 3.2 `tests/api/tools/seo.py` Add a new test class or individual test functions. The existing tests are async; new tests should also be async and use the test client. Test cases to add: | Test Name | Input URL | Expected HTTP Status | |---|---|---| | `test_run_rejects_malformed_scheme` | `"ahttps://example.com"` | 422 | | `test_run_rejects_relative_path` | `"/feed"` | 422 (or 200 with normalization – see note) | | `test_run_accepts_missing_scheme` | `"example.com"` | 200 (with normalized URL `https://example.com`) | | `test_run_rejects_non_http_scheme` | `"ftp://example.com"` | 422 | **Note on relative paths**: The investigation findings note that relative paths like `"/feed"` currently get `https://` prepended, becoming `"https:///feed"`, which is invalid and would fail later. To maintain consistency, we should reject them as invalid (since they are not absolute URLs). The validator above will catch them because `SEO_URL_PATTERN` requires a hostname after the scheme. The test should therefore expect 422. **Pseudo-code for test**: ```python @pytest.mark.asyncio async def test_run_rejects_malformed_scheme(seo_client, seo_payload): payload = seo_payload.copy() payload["url"] = "ahttps://devplace.net/sitem" response = await seo_client.post("/tools/seo/run", json=payload) assert response.status_code == 422 # Optionally verify error detail contains "URL" or similar @pytest.mark.asyncio async def test_run_rejects_relative_path(seo_client, seo_payload): payload = seo_payload.copy() payload["url"] = "/feed" response = await seo_client.post("/tools/seo/run", json=payload) assert response.status_code == 422 @pytest.mark.asyncio async def test_run_accepts_missing_scheme(seo_client, seo_payload): payload = seo_payload.copy() payload["url"] = "example.com" response = await seo_client.post("/tools/seo/run", json=payload) assert response.status_code == 200 data = response.json() # Verify the normalized URL was used in the enqueued job assert data.get("url") == "https://example.com" # Adjust field name if necessary @pytest.mark.asyncio async def test_run_rejects_non_http_scheme(seo_client, seo_payload): payload = seo_payload.copy() payload["url"] = "ftp://example.com" response = await seo_client.post("/tools/seo/run", json=payload) assert response.status_code == 422 ``` ### 4. Avoiding Past Failures - **Test failure**: Ensure the new tests do not interfere with existing tests (use isolated fixtures if needed). The test command runs `python -m pytest {test_files}`; we must only run the SEO-related test file to avoid side effects. The `test_command` in the prompt refers to a specific test file – we must respect that. - **Lint failure**: After making changes, run the project's lint tool (likely `ruff` or `flake8`) and fix any new violations. The previous lint failure was from hidden `.venv` files being committed; this is likely a merge conflict issue, not a style issue. We must ensure we do not introduce new violations in the source code. - **Merge conflict**: The merge conflict appeared from rebasing onto base branch when `.venv` was tracked. We must ensure our branch is based on the correct base and does not include `.venv` files. The plan itself assumes a clean checkout. ### 5. Definition of Done - [ ] In `app/core/models.py`, add the `SEO_URL_PATTERN` constant and the `field_validator` to `SeoRunForm` as described. - [ ] In `tests/api/tools/seo.py`, add the four test functions listed above. - [ ] Run the project's test command: `pip install -e '.[dev]' -q && python -m pytest tests/api/tools/seo.py` – **all tests pass**, including the new ones and the existing five. - [ ] Run the project's lint command (e.g., `ruff check app/ tests/` or similar as defined in `Makefile` or `pyproject.toml`) – **no new lint violations** are introduced. - [ ] Verify that no `.venv` or other unintended artifacts are in the branch diff. - [ ] Confirm that the validator rejects the ticket's example URL `"ahttps://devplace.net/sitem"` with a 422 response in the test run. Opened automatically by Typosaurus.

typosaurus added 1 commit 2026-07-23 04:38:04 +02:00