feat: migrate KSeF client from API v1 to v2#34
Conversation
The KSeF production API v1 (/api/online on ksef.mf.gov.pl) has been shut down, returning HTML instead of JSON and causing AuthWorker crashes. Key changes: - Base URLs: ksef-test.mf.gov.pl → api-test.ksef.mf.gov.pl (test), ksef.mf.gov.pl → api.ksef.mf.gov.pl (prod) - Path prefix: /api/online → /v2 - Auth header: custom SessionToken → standard Authorization: Bearer - Auth flow: operation_token → auth_token with nested token objects - XML namespace: v1 AuthorisationChallengeRequest → v2 AuthTokenRequest with certificateSubject identifier type - Query format: flat timestamp thresholds → nested dateRange with PermanentStorage type, pagination moved to query params - terminate_session: GET → DELETE /auth/sessions/current - HTML response guards on all endpoints to prevent FunctionClauseError Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughMigrates KSeF integration to API v2: updates env/config URLs, refactors REST client to v2 endpoints and Bearer auth, renames operation_token→auth_token across behaviour/implementation/tests, updates XAdES AuthTokenRequest XML, enhances xmlsec1 handling, changes sync worker cancellation semantics, and updates tests and lockfile. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Signer as XAdESSigner
participant API as KSeF_API
Client->>API: POST /v2/auth/challenge (empty JSON)
API-->>Client: 200 {reference_number, challenge}
Client->>Signer: sign challenge -> signed_xml
Signer-->>Client: signed_xml
Client->>API: POST /v2/auth/xades-signature (signed_xml)
API-->>Client: 202 {reference_number, auth_token, auth_token_valid_until}
loop poll
Client->>API: GET /v2/auth/{reference} (Authorization: Bearer auth_token)
API-->>Client: 200 {status: pending|success|error, ...}
end
alt success
Client->>API: POST /v2/auth/token/redeem (Authorization: Bearer auth_token)
API-->>Client: 200 {access_token, refresh_token, access_valid_until, refresh_valid_until}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/ksef_hub/ksef_client/live.ex (2)
148-184:⚠️ Potential issue | 🟠 MajorUse RateLimiter to enforce proactive rate limiting instead of only reacting to 429 responses.
The client currently lacks proactive rate limiting. A
RateLimitermodule already exists in the codebase with built-in support for operation-type-specific rate windows. CallRateLimiter.wait_for_slot(:metadata)before theReq.postrequest to enforce the query rate limit before making the request, rather than waiting for the server to reject it.
187-205:⚠️ Potential issue | 🟠 MajorAdd rate limiting to invoice downloads (8 req/s).
Thedownload_invoicefunction is called in a tight loop without throttling, which can exceed KSeF's recommended rate limits. AddProcess.sleep(125)before the HTTP request to throttle to 8 req/s.🔧 Suggested change
def download_invoice(access_token, ksef_number) do url = api_url("/invoices/ksef/#{ksef_number}") headers = bearer_headers(access_token) + + Process.sleep(125) case Req.get(url, headers: headers) do
🧹 Nitpick comments (4)
lib/ksef_hub/ksef_client/live.ex (2)
9-15: Add@doc/@SPEC for public functions and@specfor base_url/api_url/bearer_headers.
These functions are missing required docs/specs in this module.As per coding guidelines “Every public function must have
@docdocumentation” and “Every function (public and private) must have@spectype specification.”Also applies to: 17-37, 40-63, 66-90, 93-119, 122-145, 148-184, 187-205, 208-217
221-253: Consider piping the dateRange build to follow the transformation-chain guideline.
You can express the conditional map updates as a pipeline while keeping the same behavior.As per coding guidelines “Use `|>` pipelines for data transformation chains in Elixir.”♻️ Possible refactor
defp build_date_range(from, to) do - range = %{"dateType" => "PermanentStorage"} - range = if from, do: Map.put(range, "from", DateTime.to_iso8601(from)), else: range - range = if to, do: Map.put(range, "to", DateTime.to_iso8601(to)), else: range - range + [{"from", from}, {"to", to}] + |> Enum.reduce(%{"dateType" => "PermanentStorage"}, fn + {_, nil}, acc -> acc + {key, dt}, acc -> Map.put(acc, key, DateTime.to_iso8601(dt)) + end) endlib/ksef_hub/ksef_client/auth.ex (2)
28-43: Missing@specfor public functionauthenticate/3.Per coding guidelines, every function (public and private) must have a
@spectype specification.🔧 Proposed fix
+ `@spec` authenticate(String.t(), binary(), String.t()) :: + {:ok, + %{ + access_token: String.t(), + refresh_token: String.t(), + access_valid_until: DateTime.t(), + refresh_valid_until: DateTime.t() + }} + | {:error, term()} def authenticate(nip, certificate_data, certificate_password) doAs per coding guidelines: "Every function (public and private) must have
@spectype specification."
45-64: Missing@specfor private functionpoll_until_ready/3.Per coding guidelines, every function (public and private) must have a
@spectype specification.🔧 Proposed fix
+ `@spec` poll_until_ready(String.t(), String.t(), non_neg_integer()) :: :ok | {:error, term()} defp poll_until_ready(reference_number, auth_token, attempt \\ 0)As per coding guidelines: "Every function (public and private) must have
@spectype specification."
…t found
- SyncWorker now returns {:cancel, reason} instead of silently returning
:ok when credential or certificate is missing. This makes the failure
visible in oban_jobs state and logs.
- XadesSigner.Xmlsec1 now catches :enoent from System.cmd and returns a
clear error message instead of crashing with a raw EXIT.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/ksef_hub/xades_signer/xmlsec1.ex (1)
15-16: 🛠️ Refactor suggestion | 🟠 MajorMissing
@docand@specfor public function.The
sign_challenge/4function is a public callback but lacks the required documentation and type specification.Proposed fix
+ `@doc` """ + Signs an authentication challenge using xmlsec1 CLI with the provided certificate. + Returns the signed XML on success or an error tuple on failure. + """ + `@spec` sign_challenge(String.t(), String.t(), binary(), String.t()) :: + {:ok, String.t()} | {:error, term()} `@impl` true def sign_challenge(challenge, nip, certificate_data, certificate_password) doAs per coding guidelines: "Every public function must have
@docdocumentation" and "Every function (public and private) must have@spectype specification".
🧹 Nitpick comments (1)
lib/ksef_hub/xades_signer/xmlsec1.ex (1)
57-59: Misleading exit code0in error tuple.Using
0as the exit code suggests success, but this branch handles a failure case. Consider using a distinct sentinel value or omitting the exit code field entirely for this error type.Proposed fix
{:ok, {:error, %ErlangError{original: reason}}} -> Logger.error("xmlsec1 failed to start: #{inspect(reason)}") - {:error, {:xmlsec1_failed, 0, inspect(reason)}} + {:error, {:xmlsec1_start_failed, inspect(reason)}}
xmlsec1 1.3+ (xmlsec library 3.x, installed via Homebrew) removed the --pwd-file flag. Detect version at runtime and use --pwd for >= 1.3, --pwd-file for older versions (Linux/Docker). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/ksef_hub/xades_signer/xmlsec1.ex (1)
17-73: 🛠️ Refactor suggestion | 🟠 MajorMissing
@docand function exceeds complexity threshold.
- Per coding guidelines, every public function must have
@docdocumentation.- The pipeline correctly flags cyclomatic complexity at 10 (max 9). The function handles multiple concerns: temp file setup, task execution, result pattern matching, and error handling.
Consider extracting the task execution and result handling:
♻️ Proposed refactor to reduce complexity
+ `@doc` """ + Signs the authentication challenge using xmlsec1 CLI with the provided PKCS12 certificate. + + Returns `{:ok, signed_xml}` on success, or `{:error, reason}` on failure. + """ `@spec` sign_challenge(String.t(), String.t(), binary(), String.t()) :: {:ok, String.t()} | {:error, term()} def sign_challenge(challenge, nip, certificate_data, certificate_password) do xml_template = build_auth_token_request(challenge, nip) cert_path = SecureTemp.write(certificate_data, "cert.p12") xml_path = SecureTemp.write(xml_template, "request.xml") signed_path = SecureTemp.path("signed.xml") {password_args, password_path} = build_password_args(certificate_password) try do args = ["--sign", "--pkcs12", cert_path] ++ password_args ++ ["--output", signed_path, xml_path] - task = - Task.async(fn -> - try do - System.cmd("xmlsec1", args, stderr_to_stdout: true) - rescue - e in ErlangError -> - {:error, e} - end - end) - - case Task.yield(task, 30_000) || Task.shutdown(task, :brutal_kill) do - {:ok, {_output, 0}} -> - {:ok, File.read!(signed_path)} - - {:ok, {:error, %ErlangError{original: :enoent}}} -> - Logger.error( - "xmlsec1 not found. Install it: brew install xmlsec1 (macOS) or apt-get install xmlsec1 (Linux)" - ) - - {:error, {:xmlsec1_not_found, "xmlsec1 binary not found in PATH"}} - - {:ok, {:error, %ErlangError{original: reason}}} -> - Logger.error("xmlsec1 failed to start: #{inspect(reason)}") - {:error, {:xmlsec1_failed, 0, inspect(reason)}} - - {:ok, {output, exit_code}} -> - Logger.error("xmlsec1 failed (exit #{exit_code}): #{output}") - {:error, {:xmlsec1_failed, exit_code, output}} - - nil -> - Logger.error("xmlsec1 timed out after 30s") - {:error, :timeout} - end + args + |> run_xmlsec1_with_timeout() + |> handle_xmlsec1_result(signed_path) after cleanup_paths = [cert_path, xml_path, signed_path] cleanup_paths = if password_path, do: [password_path | cleanup_paths], else: cleanup_paths Enum.each(cleanup_paths, &SecureTemp.delete/1) end end + + `@spec` run_xmlsec1_with_timeout([String.t()]) :: {:ok, term()} | nil + defp run_xmlsec1_with_timeout(args) do + task = + Task.async(fn -> + try do + System.cmd("xmlsec1", args, stderr_to_stdout: true) + rescue + e in ErlangError -> {:error, e} + end + end) + + Task.yield(task, 30_000) || Task.shutdown(task, :brutal_kill) + end + + `@spec` handle_xmlsec1_result({:ok, term()} | nil, String.t()) :: + {:ok, String.t()} | {:error, term()} + defp handle_xmlsec1_result({:ok, {_output, 0}}, signed_path), do: {:ok, File.read!(signed_path)} + + defp handle_xmlsec1_result({:ok, {:error, %ErlangError{original: :enoent}}}, _signed_path) do + Logger.error("xmlsec1 not found. Install it: brew install xmlsec1 (macOS) or apt-get install xmlsec1 (Linux)") + {:error, {:xmlsec1_not_found, "xmlsec1 binary not found in PATH"}} + end + + defp handle_xmlsec1_result({:ok, {:error, %ErlangError{original: reason}}}, _signed_path) do + Logger.error("xmlsec1 failed to start: #{inspect(reason)}") + {:error, {:xmlsec1_failed, 0, inspect(reason)}} + end + + defp handle_xmlsec1_result({:ok, {output, exit_code}}, _signed_path) do + Logger.error("xmlsec1 failed (exit #{exit_code}): #{output}") + {:error, {:xmlsec1_failed, exit_code, output}} + end + + defp handle_xmlsec1_result(nil, _signed_path) do + Logger.error("xmlsec1 timed out after 30s") + {:error, :timeout} + end
🤖 Fix all issues with AI agents
In `@lib/ksef_hub/xades_signer/xmlsec1.ex`:
- Around line 75-85: The build_password_args/1 function currently passes the
password on the command line when supports_pwd_file?() is false, exposing
secrets; update this by documenting the risk in the module moduledoc, change
build_password_args/1 to prefer writing password to a temp file with secure
permissions via SecureTemp.write (and return the file path) when possible, and
add a fallback strategy: detect if the installed xmlsec1 supports stdin password
(e.g., `--pwd -`) and use stdin-based password passing instead of `--pwd` when
available; also add guidance in the moduledoc recommending pinning xmlsec1 < 1.3
in controlled images and instruct operators to ensure temp files are created
with 0600 permissions and securely removed.
🧹 Nitpick comments (1)
lib/ksef_hub/xades_signer/xmlsec1.ex (1)
87-104: Consider caching version detection and documenting fallback behavior.Two observations:
Repeated syscalls:
supports_pwd_file?/0spawnsxmlsec1 --versionon every signing operation. Since the installed version won't change at runtime, consider memoizing the result (e.g., via:persistent_termor module attribute at compile time if acceptable).Fallback defaults to insecure: When version parsing fails (lines 95-96, 99-100, 102-103), the function returns
false, causing--pwdusage. This is the less secure path. Consider logging a warning when falling back so operators are aware.♻️ Optional: Add warning on fallback
[_, major, minor] -> {String.to_integer(major), String.to_integer(minor)} < {1, 3} _ -> + Logger.warning("Could not parse xmlsec1 version, falling back to --pwd") false end _ -> + Logger.warning("xmlsec1 --version failed, falling back to --pwd") false end rescue - _ -> false + _ -> + Logger.warning("xmlsec1 version check raised, falling back to --pwd") + false end
| # xmlsec1 >= 1.3 (xmlsec library 3.x) removed --pwd-file, only --pwd is available. | ||
| # Older versions support --pwd-file for secure file-based password passing. | ||
| @spec build_password_args(String.t()) :: {[String.t()], String.t() | nil} | ||
| defp build_password_args(password) do | ||
| if supports_pwd_file?() do | ||
| path = SecureTemp.write(password, "password.txt") | ||
| {["--pwd-file", path], path} | ||
| else | ||
| {["--pwd", password], nil} | ||
| end | ||
| end |
There was a problem hiding this comment.
Security concern: password passed as CLI argument for xmlsec1 >= 1.3.
Line 83 passes the certificate password directly via --pwd, which exposes it in process listings (ps aux), /proc/<pid>/cmdline, and potentially system audit logs. This violates the security guidance to "never pass passwords as CLI arguments."
I understand xmlsec1 >= 1.3 removed --pwd-file, but consider:
- Document the risk in the moduledoc so operators can make informed deployment decisions.
- Pin xmlsec1 < 1.3 in Docker images where you control the environment.
- Explore stdin passing — some xmlsec1 builds accept password via stdin with
--pwd -(verify with your target version).
📝 Suggested documentation update
`@moduledoc` """
Production XADES signer using xmlsec1 CLI.
Only called during initial auth and re-auth (~every 48 days).
Uses secure temp files with 0600 permissions, cleaned up after use.
Supports both xmlsec1 < 1.3 (has --pwd-file) and >= 1.3 (only --pwd).
+
+ ## Security Note
+
+ xmlsec1 >= 1.3 removed the `--pwd-file` option, requiring passwords to be
+ passed via `--pwd` CLI argument. This exposes the password in process listings.
+ For production deployments, consider using xmlsec1 < 1.3 or ensuring the
+ environment restricts access to process information.
"""Based on learnings: "When calling xmlsec1 for XADES signing: write cert/password to temp files with 0600 permissions, never pass passwords as CLI arguments."
🤖 Prompt for AI Agents
In `@lib/ksef_hub/xades_signer/xmlsec1.ex` around lines 75 - 85, The
build_password_args/1 function currently passes the password on the command line
when supports_pwd_file?() is false, exposing secrets; update this by documenting
the risk in the module moduledoc, change build_password_args/1 to prefer writing
password to a temp file with secure permissions via SecureTemp.write (and return
the file path) when possible, and add a fallback strategy: detect if the
installed xmlsec1 supports stdin password (e.g., `--pwd -`) and use stdin-based
password passing instead of `--pwd` when available; also add guidance in the
moduledoc recommending pinning xmlsec1 < 1.3 in controlled images and instruct
operators to ensure temp files are created with 0600 permissions and securely
removed.
xmlsec1 requires a <ds:Signature> skeleton in the XML template to know where to place the signature. Without it, signing fails with "failed to find default node with name=Signature". Also reverts temporary debug logging from xmlsec1.ex. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract run_xmlsec1/1 and handle_result/2 helpers to satisfy credo --strict complexity threshold. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
The KSeF production API v1 (/api/online on ksef.mf.gov.pl) has been shut down, returning HTML instead of JSON and causing AuthWorker crashes.
Key changes:
Summary by CodeRabbit
New Features
Improvements
Behavior Changes
Configuration
Tests