Correct a typo, also enforce host being a required header #3

Merged
floatingghost merged 3 commits from typo into main 2026-05-04 18:57:12 +00:00
No description provided.
@ -260,1 +260,4 @@
with true <- !has_body || uses_header?("digest", used_headers, header_defs),
true <-
uses_header?("host", used_headers, header_defs) ||
uses_header?(:host, used_headers, header_defs),
Owner

later signature string generation will only read "host" and afaict actual headers in a plug are guaranteed to always be binaries anyway, so only the first check should be needed

later signature string generation will only read `"host"` and afaict actual headers in a plug are [guaranteed to always be binaries](https://hexdocs.pm/plug/Plug.Conn.html#t:headers/0) anyway, so only the first check should be needed
Author
Owner

the pure reason for the double check is this test which passes it as an atom - i wasn't sure if this was fully intentional for backwards compat or similar, so checked both

which should be fine i think?

the pure reason for the double check is [this test](https://akkoma.dev/AkkomaGang/http_signatures/src/branch/main/test/http_signatures_test.exs#L194) which passes it as an atom - i wasn't sure if this was fully intentional for backwards compat or similar, so checked both which should be fine i think?
Owner

this test

oh, this seems to just be an artefact from the old API. It used to pass all header parameters, except request-target for the sign function as atoms. The current API expects it to be all binaries, so as it is called now it actually ends up generating a borked signature. Must have forgotten to change the type here for some reason no, i was thinking of the verification processing. It might technically work, but is breaking the documented API contract

@spec sign(HTTPKey.t(), String.t(), %{String.t() => String.t()}, Keyword.t()) :: String.t()
> this test oh, this seems to just be an artefact from the old API. It used to pass all header parameters, except `request-target` for the `sign` function as atoms. The current API expects it to be all binaries, so as it is called now ~~it actually ends up generating a borked signature. Must have forgotten to change the type here for some reason~~ no, i was thinking of the verification processing. It might _technically_ work, but is breaking the documented API contract ```elixir @spec sign(HTTPKey.t(), String.t(), %{String.t() => String.t()}, Keyword.t()) :: String.t() ```
Author
Owner

alright, I'll swap it over to string-only

alright, I'll swap it over to string-only
Oneric marked this conversation as resolved
@ -377,2 +386,3 @@
{"date", "Sun, 11 Mar 2018 12:19:36 GMT"},
{"digest", "SHA-256=V7Hl6qDK2m8WzNsjzNYSBISi9VoIXLFlyjF/a5o1SOc="}
{"digest", "SHA-256=V7Hl6qDK2m8WzNsjzNYSBISi9VoIXLFlyjF/a5o1SOc="},
{"host", "example.com"}
Owner

nit: this and the previous test already fail before it becomes relevant anyway, but due to adding host to headers this header is not just missing the key (or for the test above "too old"), but also plain cryptographically incorrect (the current value did not actually sign the hostheader content)

nit: this and the previous test already fail before it becomes relevant anyway, but due to adding `host` to `headers` this header is not just missing the key (or for the test above "too old"), but also plain cryptographically incorrect (the current value did not actually sign the `host`header content)
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
AkkomaGang/http_signatures!3
No description provided.