Skip to content

fix(hmac): validate empty-payload POSTs, and repair the integration suite - #123

Merged
MarTrepodi merged 10 commits into
developfrom
fix/integration-items-param
Aug 3, 2026
Merged

fix(hmac): validate empty-payload POSTs, and repair the integration suite#123
MarTrepodi merged 10 commits into
developfrom
fix/integration-items-param

Conversation

@MarTrepodi

@MarTrepodi MarTrepodi commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

The Integration Tests workflow has been red on every branch since 2026-07-26, including on main. This fixes a real HMAC bug found along the way, hardens the suite, and quarantines the one failure that is not ours to fix.

The /data failure is upstream, not a parameter problem

Comlink returns HTTP 400 with:

Did not receive a response code back from the server, even after a retry. The "items" parameter value "8" may be invalid. Try one of the GameDataItems values from /enums.

The trailing hint reads like a deprecation notice, but it is boilerplate appended whenever Comlink's own upstream fetch fails. Four CI runs show the hint is not the cause:

image /data call result
4.4.1 request_segment=1 400
4.4.1 items=2097151 — Segment1, read from that server's own /enums 400
4.4.1 items=8 — EquipmentDefinitions 400
4.4.0 items=8 400

Three parameter forms, two image versions, all failing identically while the other 27 tests pass. items=8 is unambiguously valid and returns in 0.6s against a warm instance. Since 4.4.0 is what latest resolved to during the green period ending 2026-07-19 and it fails now too, the change is upstream of this repository — a cold service container can no longer complete a /data fetch from the game servers.

Both /data tests are therefore marked xfail, non-strict: they pass against a warm instance, so an XPASS is the signal that upstream has recovered and the marker can be removed.

HMAC: empty-payload POSTs never validated

_construct_request_headers tested payload truthiness, so an empty dict hashed "" while httpx transmitted {}:

payload={}  wire=b'{}'  digest_matches=False

Every HMAC-authenticated POST with an empty payload was rejected with HTTP 403 HMACValidationError — including get_game_metadata() with no client_specs, the common case. The fix is one condition: hash the payload whenever a body is sent, and reserve the empty string for bodiless requests. CI confirms it — the two HMAC failures are gone and the passing count went 25 → 27.

This was invisible because the two tests covering it called get_enums(), a GET endpoint Comlink does not authenticate. They now issue a signed POST.

Assertion audit

/data always returns the full set of collection keys regardless of what was requested; unrequested collections come back empty. So len(result) > 0 passed even if the filter matched nothing. The same looseness ran through the suite:

  • 4 HMAC rejection tests passed against an unreachable server. SwgohComlinkException wraps transport errors as well as HTTP status errors, so pytest.raises(SwgohComlinkException) was satisfied by "connection refused" — if the comlink-hmac service failed to start, they reported green having tested nothing. Now matched against HTTP 4\d{2}; CI confirms the rejection status is 403.
  • 2 HMAC "success" tests did not exercise HMAC — see above.
  • 2 context-manager tests asserted nothing about the context manager. They now assert the underlying HTTP client is open inside the block and closed after.
  • Empty-value assertions tightened for version strings, metadata, pvpProfile, and gameEvent — each previously passed on empty.
  • New coverage for player_details_only, used in four tests but never verified. It keeps the arena tabs and nulls out squad.

Also

Both service images are pinned to 4.4.1 instead of latest, so an upstream release can no longer turn the suite red with no commit of ours. Bump the tag deliberately.

🤖 Generated with Claude Code

dependabot Bot and others added 6 commits July 28, 2026 08:30
Bumps [pymdown-extensions](https://github.com/facelessuser/pymdown-extensions) from 10.21.3 to 11.0.
- [Release notes](https://github.com/facelessuser/pymdown-extensions/releases)
- [Commits](facelessuser/pymdown-extensions@10.21.3...11.0)

---
updated-dependencies:
- dependency-name: pymdown-extensions
  dependency-version: '11.0'
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps
[pymdown-extensions](https://github.com/facelessuser/pymdown-extensions)
from 10.21.3 to 11.0.
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a
href="https://github.com/facelessuser/pymdown-extensions/releases">pymdown-extensions's
releases</a>.</em></p>
<blockquote>
<h2>11.0</h2>
<ul>
<li><strong>BREAK</strong>: B64: Restricts relative links to
<code>base_path</code> by default. Can be disabled by setting new
<code>restrict_path</code>
option to <code>False</code>. The new <code>root_path</code> can be
specified if paths are desired to be restricted to a different
location separate <code>base_path</code> which is also used as a
relative base for image paths.</li>
<li><strong>NEW</strong>: Drop Python 3.9 support.</li>
<li><strong>FIX</strong>: Tabbed: Fix issue where an empty title would
cause an exception.</li>
</ul>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a
href="https://github.com/facelessuser/pymdown-extensions/commit/6d0097b85a52468e7cd8eaceaa192ff3348c41e0"><code>6d0097b</code></a>
Update changelog</li>
<li><a
href="https://github.com/facelessuser/pymdown-extensions/commit/f4808ea6c408662cf5c542809cc07d87004b9b99"><code>f4808ea</code></a>
Bump version and update changelog</li>
<li><a
href="https://github.com/facelessuser/pymdown-extensions/commit/a290074f72e885cf123cff0f9e6442856c19020b"><code>a290074</code></a>
Update JS doc deps</li>
<li><a
href="https://github.com/facelessuser/pymdown-extensions/commit/edce35586d11a1ef78bb187bc60497fe6dbf3b64"><code>edce355</code></a>
Merge commit from fork</li>
<li><a
href="https://github.com/facelessuser/pymdown-extensions/commit/9fb9643c84c46386d77eef1a18929330620e3111"><code>9fb9643</code></a>
Update Python type deps</li>
<li><a
href="https://github.com/facelessuser/pymdown-extensions/commit/299a1294f3d500c05d7aee1c318b1836764c773b"><code>299a129</code></a>
Docs: Update JS deps</li>
<li><a
href="https://github.com/facelessuser/pymdown-extensions/commit/367d965941055f3196121833d84070401ea1ab8f"><code>367d965</code></a>
Drop Python 3.9 (<a
href="https://redirect.github.com/facelessuser/pymdown-extensions/issues/2902">#2902</a>)</li>
<li><a
href="https://github.com/facelessuser/pymdown-extensions/commit/19ec93690bd3a038c1eddc4fe63850e20daf3bb4"><code>19ec936</code></a>
Fix issue with empty titles and Tabbed (<a
href="https://redirect.github.com/facelessuser/pymdown-extensions/issues/2901">#2901</a>)</li>
<li><a
href="https://github.com/facelessuser/pymdown-extensions/commit/4868783ce224ed61abd9abcafd098015ca3c65a3"><code>4868783</code></a>
Fix npm option</li>
<li><a
href="https://github.com/facelessuser/pymdown-extensions/commit/18a21f55875a4cfc4f689d49009696fc6f6bbddc"><code>18a21f5</code></a>
zensical config should be stored in sdist</li>
<li>Additional commits viewable in <a
href="https://github.com/facelessuser/pymdown-extensions/compare/10.21.3...11.0">compare
view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=pymdown-extensions&package-manager=uv&previous-version=10.21.3&new-version=11.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)
You can disable automated security fix PRs for this repo from the
[Security Alerts
page](https://github.com/swgoh-utils/comlink-python/network/alerts).

</details>
@github-actions github-actions Bot added ci Changes related to continuous integration processes dependencies Updates to package dependency libraries testing labels Aug 3, 2026
@github-actions github-actions Bot added the code label Aug 3, 2026
@MarTrepodi MarTrepodi changed the title fix(tests): repair and harden the integration suite fix(hmac): validate empty-payload POSTs, and repair the integration suite Aug 3, 2026
@MarTrepodi
MarTrepodi merged commit e821288 into develop Aug 3, 2026
15 checks passed
@MarTrepodi
MarTrepodi deleted the fix/integration-items-param branch August 3, 2026 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci Changes related to continuous integration processes code dependencies Updates to package dependency libraries testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant