Skip to content

alphanumeric() validator accepts trailing newline (sibling of #39234, #39548) #39666

Description

@Harsh23Kashyap

alphanumeric() validator in api/libs/helper.py accepts a trailing newline

libs.helper.alphanumeric uses re.match(r"^[a-zA-Z0-9_]+$", value). In Python $ matches at end-of-string OR just before a final \n, so a value like "my_tool\n" slips through and is returned to the caller unchanged. The validator's contract says "alphanumeric and underscore only" — the actual behaviour is "alphanumeric, underscore, and one optional trailing newline".

Where it matters

alphanumeric is the field validator for WorkflowToolBasePayload.name (api/controllers/console/workspace/tool_providers.py:183). Both WorkflowToolCreatePayload and WorkflowToolUpdatePayload use it. The tool name is stored in the database, rendered in the console UI, and used to look the tool up later, so a tool named my_tool and a tool named my_tool\n are distinct rows that look identical to a human reader.

Reproduction

>>> from libs.helper import alphanumeric
>>> alphanumeric("my_tool")
'my_tool'
>>> alphanumeric("my_tool\n")
'my_tool\n'      # accepted; should be rejected
>>> alphanumeric("my_tool\r\n")
ValueError: ...   # rejected (the \r is not alphanumeric, but the \n would be)

The first three lines are the bug; the third line is the asymmetry that shows the validator is rejecting on the wrong rule.

Why it matters in practice

  • A second user can create a tool whose name visually matches an existing one but is a different database row. The two are not deduplicated and not interchangeable, but they look the same in lists and logs.
  • A name with a trailing \n lands in the database as-is. Anywhere that name is concatenated into a log line or a downstream protocol message (HTTP header, JSON value, terminal escape) becomes a log-injection / header-injection vector. The fix in email() validator accepts a trailing newline (mail header-injection vector on the auth surface) #39234 closed the same vector for the email validator; the same fix is missing here.
  • The validator comment on line 291 says "alphanumeric and underlined". The trailing-newline acceptance contradicts that comment.

Fix

Replace re.match with re.fullmatch at api/libs/helper.py:292, and add a one-line comment matching the style of the email validator fix at helper.py:224:

def alphanumeric(value: str):
    # check if the value is alphanumeric and underlined
    # Use re.fullmatch instead of re.match to reject trailing newlines.
    # In Python, '$' matches at end-of-string OR just before a trailing newline,
    # so re.match accepts "tool_name\n". re.fullmatch requires the entire
    # string to match.
    if re.fullmatch(r"^[a-zA-Z0-9_]+$", value):
        return value

    raise ValueError(f"{value} is not a valid alphanumeric value")

Add a test in api/tests/unit_tests/libs/test_helper.py covering alphanumeric("tool\n") raises ValueError, and the existing happy-path inputs still pass.

Scope

  • One source line in api/libs/helper.py.
  • One new test function in api/tests/unit_tests/libs/test_helper.py.
  • No API change, no schema change, no migration. Existing rows are unaffected (a name ending in \n is invalid by both old and new contract; nothing needs rewriting).

Acceptance criteria

  • alphanumeric("tool_name") returns "tool_name" (unchanged).
  • alphanumeric("tool_name\n") raises ValueError.
  • alphanumeric("tool_name\r") raises ValueError (already did).
  • All existing tests pass; new test added.
  • make lint and make test TARGET_TESTS=./api/tests/unit_tests/libs/test_helper.py are clean.

Out of scope (left for separate issues)

The same re.match-with-trailing-newline pattern appears in two more places in api/libs/:

  • api/libs/custom_inputs.py:26parse_time_duration accepts "7d\n".
  • api/libs/time_parser.py:28 — time-parser helper accepts the same.

These are real but lower-impact than the alphanumeric case (the inputs are usually form-supplied; the validators don't gate a security surface as directly as WorkflowToolBasePayload.name). Filing them as follow-ups if maintainers want a sweep, but keeping this issue scoped to the highest-impact single call site.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions