← Community
bugopen

plan_pin.manifest_facets keys by command id, not provider_verb. #310 module update guard is silently inert for modules declaring a provider

SA
sakrit2041h ago · 14 views
affected: station-v1.5.40

Reproduction steps

  1. Install a module whose commands declare a per command provider, or whose

command ids contain no dot. Two modules shipped with the station already do:
sami666-google-sheets on all three of its commands, and
sami666-railcall-essentials on all three of its commands.

  1. Build a workflow that uses one of those commands and approve it for live

runs, so plan_pin.pin() records the approval.

  1. Open the pin under .railcall_workspace/plan_pins/. It has no

manifest_facets key.

  1. Edit that module's module.json the way a module update would: flip a

command's mode from read to write, set risk to high, widen
allowed_destinations to *, widen credential_spec. Leave the workflow
untouched, so the plan root is unchanged.

  1. Run the workflow live. It runs.

repro/pin_facets_repro.py in the report below does all of this against the
station's own plan_pin.py in a throwaway directory, and prints a control case
alongside so the difference is visible. It reproduces on station-v1.5.26 and on
station-v1.5.40, and identically under Ubuntu on WSL with Python 3.12.3.

Expected behavior

The guard refuses the run and asks for re-approval. The file states the intent
plainly: publisher signatures own code trust, but the manifest is the surface a
human's approval was about, so a module update that widens mode, risk,
allowed_destinations or credential_spec must trip re-approval even when the plan
root is unchanged.

Actual behavior

manifest_facets() builds its lookup keyed by the command id with dots
replaced:

idx[cid.replace(".", "_")] = fp

then matches that against each node's action_id:

aid = n.get("action_id")
if aid in idx:
    out[aid] = idx[aid]

action_id is not the command id with its dots replaced. routes/modules.py
builds it in _module_provider_verb, joined in _module_action_row as
f"{provider}_{verb}":

parts = cid.split(".", 1)
provider = manifest_cmd.get("provider") or (parts[0] if len(parts) == 2 else slug)
verb_raw = parts[1] if len(parts) == 2 else cid
return provider, verb_raw.replace(".", "_")

The two agree only when there is no provider override and the id has exactly
one dot:

| command | engine action_id | guard lookup key |
|---|---|---|
| users.find with provider: "acme" | acme_find | users_find |
| sync, no dot, slug acme-tool | acme-tool_sync | sync |

When they disagree nothing matches, out stays empty, and the function returns
None. pin() writes a pin with no manifest_facets, and check() treats a
pin without facets as an old pin and skips the comparison:

pinned_facets = rec.get("manifest_facets")
if pinned_facets and current_facets is not None:

The version gating that exists to avoid a fleet wide re-approval wave is what
hides the failure. Nothing warns at pin time or at run time, and check()
returns "matches the approved plan".

Repro output, control first:

== control: no provider override, one dot
   engine action_id : users_find
   guard lookup key : users_find
   facets pinned    : 2
   run allowed      : False   (a module update changed the governed surface)
   GUARD FIRED

== a per command provider override
   engine action_id : acme_find
   guard lookup key : users_find
   facets pinned    : none
   pin has the field: False
   run allowed      : True   (matches the approved plan)
   GUARD SILENT

== a command id with no dot
   engine action_id : acme-tool_sync
   guard lookup key : sync
   facets pinned    : none
   run allowed      : True   (matches the approved plan)
   GUARD SILENT

The control is the point. The guard is correctly implemented and correctly
wired. The only thing wrong is the key it looks up, so this is not a broken
feature but a feature that silently exempts a class of modules.

The live path uses this. routes/dispatch_workflow.py computes
current_facets and calls check() before any effect fires, refusing with
gate="plan_pin_changed". For a diverging module it never refuses.

Honest scope and trigger

No local file tampering is needed, which is what separates this from 116992.
The exemption is earned by provider, a documented manifest field with no
security meaning attached to it, so a module author can become permanently
exempt from this guard without doing anything that looks like an attack.

Sixteen of the eighteen modules installed here happen to align, which is why
the guard has looked like it works. The two that diverge are both yours, and
one of them writes.

I have not checked whether the marketplace refuses a module update that widens
mode or allowed_destinations at publish time. If it does, the path narrows
to locally installed and side loaded modules, but it does not close, and the
guard is still reporting success while inert.

I also have not built a repro for a related thing worth a look while the file
is open: idx is one flat namespace across every installed module, so two
modules declaring the same provider and verb collide, and whichever is scanned
last wins.

Suggested fix

One line, plus the helper it needs.

def _engine_action_id(cid, cmd, slug):
    """The action id the workflow engine builds for a module command.

    Copied from routes/modules.py::_module_provider_verb plus the join in
    _module_action_row, so this file keys facets by the same string the nodes
    carry.
    """
    parts = cid.split(".", 1)
    provider = (cmd or {}).get("provider") or (parts[0] if len(parts) == 2 else slug)
    verb_raw = parts[1] if len(parts) == 2 else cid
    return "%s_%s" % (provider, verb_raw.replace(".", "_"))
-                idx[cid.replace(".", "_")] = fp
+                idx[_engine_action_id(cid, cmd, man.get("slug") or slug)] = fp

Verified against the shipped file, all three cases:

guard fires on: control / provider override / dotless id
  before : [True, False, False]
  after  : [True, True, True]

It causes no re-approval wave. Where the two derivations already agree the key
is unchanged, so existing pins keep matching. Where they diverge the pin carries
no facets today, so check() keeps skipping exactly as it does now until the
next re-approval. The version gating the file documents is preserved.

Sharing the derivation would be better than copying it, but primitives/
importing from routes/ inverts the layering. If you would rather move
_module_provider_verb into primitives/ and have both call it, I am happy to send that instead.

0 replies

Sign in to reply.