Skip to content

fix: [eupi] reject incomplete module config instead of crashing - #834

Closed
elhoim wants to merge 1 commit into
MISP:mainfrom
elhoim:fix/025-eupi-config-check-operator
Closed

elhoim wants to merge 1 commit into
MISP:mainfrom
elhoim:fix/025-eupi-config-check-operator

Conversation

@elhoim

@elhoim elhoim commented Aug 31, 2026 •

Copy link
Copy Markdown
Contributor

The defect

if not request.get("config") and not (request["config"].get("apikey") and request["config"].get("url")):
    misperrors["error"] = "EUPI authentication is missing"
    return misperrors

The two halves of this guard were joined with and, but they check mutually exclusive situations (config absent vs. config present-but-incomplete), so they can never both be true at once:

  • If request["config"] is absent entirely, not request.get("config") is True, but Python still evaluates the right-hand side to resolve the and, and request["config"] raises KeyError because the key doesn't exist.
  • If config is present but missing apikey and/or url, not request.get("config") is False, so the whole and short-circuits to False and the guard is skipped — execution falls through to PyEUPI(request["config"]["apikey"], ...) a few lines down, which raises KeyError there instead.

Either way the intended "friendly error" branch is unreachable, and the module raises an uncaught KeyError instead.

Impact

Any MISP user who enables the eupi expansion module without a complete config (missing config block, or an apikey/url left blank) gets an unhandled exception/HTTP 500 out of the module instead of the clear message "EUPI authentication is missing". This makes misconfiguration confusing to diagnose from the MISP UI.

The fix

Changed the operator from and to or, so the guard fires — and returns the intended error — whenever config is missing at all, or present but incomplete (missing apikey or url). The inner apikey and url check is left intact since both values are dereferenced unconditionally on the next line.

No behaviour change beyond making the existing, already-written error message reachable; no new default or contract change.

Verification

  • flake8 on misp_modules/modules/expansion/eupi.py: clean.
  • Full module test suite: 161 passed, 4 skipped, 5 subtests passed in 115.25s (0:01:55).

Found during a review of the repository; other findings are being submitted as separate PRs.

🤖 Generated with Claude Code

The credential check joins its two operands with `and`, so it only fires
when the request has no `config` key at all *and* the config lacks an
apikey or url. When `config` is absent the second operand evaluates
`request["config"]` and the module dies with a KeyError, which MISP shows
as an opaque module failure rather than the intended "EUPI authentication
is missing" error. When `config` is present but incomplete the guard
short-circuits to False and PyEUPI is constructed straight away, so a
missing apikey or url raises a KeyError on the very next line instead of
reporting the misconfiguration. Using `or` makes either condition
sufficient to return the authentication error.

Verified with flake8 on the changed file and the full test suite against
a local modules server: 161 passed, 4 skipped, 5 subtests passed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018dfYpyaSZd1nxSRLr8suj8
@adulau

adulau commented Aug 31, 2026

Copy link
Copy Markdown
Member

EUPI always requires an API key. Claude is confused.

@adulau adulau closed this Aug 31, 2026
@elhoim

elhoim commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

Sorry... (#/。\#)
But so far it is mostly good PRs/feedback right?

@adulau

adulau commented Aug 31, 2026

Copy link
Copy Markdown
Member

For the warning-lists it was easy. Now for misp-modules it's a complete different story. Testing is needed with the service and so on. So it won't easy.

@elhoim
elhoim deleted the fix/025-eupi-config-check-operator branch August 31, 2026 18:24
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.

2 participants