Skip to content

[Web] fix add/time_limited_alias silently discarding requests and validity#7345

Open
smpaz7467 wants to merge 1 commit into
mailcow:stagingfrom
smpaz7467:fix/time-limited-alias-api
Open

[Web] fix add/time_limited_alias silently discarding requests and validity#7345
smpaz7467 wants to merge 1 commit into
mailcow:stagingfrom
smpaz7467:fix/time-limited-alias-api

Conversation

@smpaz7467

Copy link
Copy Markdown

What

Three defects in POST /api/v1/add/time_limited_alias.

1. A request without description is silently discarded (the reported bug)

description was read as $_data['description'] with no guard. When a client omits
it, null is bound to spamalias.description, which is TEXT NOT NULL, so the
insert raises a PDOException.

set_exception_handler() is terminal — PHP tears down the script once the
handler returns — so process_add_return() never runs and nothing is ever echoed.
The client gets HTTP 200 with an empty body, and no alias exists. Defaulting the
field to an empty string removes the null insert.

spamalias.description is the only NOT NULL description column in the schema,
which is why the same unguarded read in add/domain and add/resource doesn't
fail — both of those columns are nullable.

2. validity never worked

if (isset($_data["validity"]) && !filter_var($_data["validity"], ...)) { error }
else { $_data["validity"] = 8760; }

When validity is present and valid, !filter_var(...) is false, so the
condition is false and the else fires — overwriting the user's value with the
default. Only invalid values were rejected; every valid one was discarded:

input before after
24 8760 24
1 8760 1
87600 8760 87600
0 / 87601 / -5 / "abc" error error
omitted 8760 8760

Nesting the range check fixes it. I'd have had to leave validity out of the spec
otherwise — documenting a parameter that provably does nothing seemed worse than
leaving it undocumented.

3. Spec gap

The spec documented only username and domain, while the code reads
description, validity and permanent too, so spec-driven clients (Swagger UI,
python-mailcow, generated SDKs) could not construct a working request. All three
are now documented.

Verification

  • php -l clean; openapi.yaml parses and exposes all five properties.
  • Reproduced the root cause against a real MariaDB using the spamalias DDL and the
    sql_mode from the issue:
    • description = NULLERROR 1048 (23000): Column 'description' cannot be null
    • description = '' → insert succeeds, row created
  • Exercised the validity branch across the table above, including boundaries.

Not addressed

The generic error handling. A DB error is still swallowed into an empty HTTP 200
for every endpoint, which is the second defect in the issue. Fixing it properly is
more than a bugfix: the handler builds array('mysql_error', $e) from the raw
PDOException, which carries the SQL statement and schema details, so echoing it to
API clients would turn a silent failure into an information disclosure. That needs
sanitising plus a decision on the status-code contract — a maintainer call, so I've
left the issue open rather than closing it.

Refs #7287

…idity

Three defects in add/time_limited_alias:

The description was read as $_data['description'] without a guard. When a
client omits it, null is bound to spamalias.description, which is TEXT NOT
NULL, so the insert raises a PDOException. The global exception handler is
terminal, so process_add_return() never echoes anything and the caller sees
HTTP 200 with an empty body while no alias was created. Default it to an
empty string instead.

The validity guard used a single condition whose else branch also caught the
success case, so every valid validity was overwritten with the 8760 hour
default and the parameter did nothing. Only invalid values were rejected.
Nest the range check so a valid value survives.

The OpenAPI spec documented only username and domain, while the code also
reads description, validity and permanent. Spec driven clients therefore
could not construct a working request. Document all three.

spamalias.description is the only NOT NULL description column in the schema,
which is why the same unguarded read in add/domain and add/resource does not
fail, both of those columns are nullable.

This does not change the generic exception handling. A database error is
still swallowed into an empty HTTP 200, and the message the handler builds
carries the raw PDOException, so surfacing it to API clients would need
sanitising first. That is left for a separate change.

Refs mailcow#7287

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant