Skip to content

fix(frontend): give a real error when the viewer has no environment key - #973

Open
omlahore wants to merge 1 commit into
phasehq:mainfrom
omlahore:fix/missing-environment-key-guard
Open

fix(frontend): give a real error when the viewer has no environment key#973
omlahore wants to merge 1 commit into
phasehq:mainfrom
omlahore:fix/missing-environment-key-guard

Conversation

@omlahore

Copy link
Copy Markdown

The problem

Eight call sites read the viewer's own environment key as data.environmentKeys[0] and destructure it straight away:

const {
  wrappedSeed: userWrappedSeed,
  wrappedSalt: userWrappedSalt,
  identityKey,
} = data.environmentKeys[0]

When the list is empty this throws Cannot destructure property 'wrappedSeed' of 'undefined' from inside an async callback. The user gets nothing useful, and at the sites without a catch the dialog simply stops responding.

Why an empty list is a normal outcome

resolve_environment_keys guards on app access but filters keys by environment and member:

if not user_can_access_app(info.context.user.userId, app.id):
    raise GraphQLError("You don't have access to this app")

filter = {"environment__app": app, "deleted_at": None}
if environment_id:
    filter["environment_id"] = environment_id
...
filter["user"] = org_member
return EnvironmentKey.objects.filter(**filter)

So holding app access does not imply holding a key for a given environment. Two paths reach that state:

  • CreateEnvironmentMutation provisions admin_keys only, so a non-admin member with app access holds no key for an environment created after they were added.
  • AddAppMemberMutation takes a client-supplied env_keys list and derives new_env_ids from it, so a member can be granted app access for a subset of environments.

The change

Adds requireEnvironmentKey alongside the other environment crypto helpers and uses it at each site:

} = requireEnvironmentKey(data.environmentKeys, env.name)

The failure now names the environment and says what to do:

You don't have a key for the Production environment, so it can't be shared. Ask an admin to grant you access to it and try again.

Behaviour is otherwise unchanged. The sites that threw still throw, with a message that means something. No catch blocks were added or removed.

Checks

  • tsc --noEmit on frontend is clean.
  • eslint on the changed files is clean.
  • prettier --check flags 5 of these files, but they fail identically on an unmodified main, so I left the formatting alone rather than bury the change in an unrelated reformat.

Eight call sites read the viewer's environment key as
`data.environmentKeys[0]` and destructure it immediately. That list is a
queryset filtered by environment and member, so an empty result is a
normal outcome rather than an error state, and destructuring it throws
"Cannot destructure property 'wrappedSeed' of 'undefined'" from inside an
async callback. The user is told nothing, and at the sites that do not
catch, the dialog just stops responding.

An empty list is reachable because the resolver guards on
`user_can_access_app` while filtering keys by environment and member:

- CreateEnvironmentMutation provisions `admin_keys` only, so a non-admin
  member with app access holds no key for an environment created after
  they were added.
- AddAppMemberMutation takes a client-supplied `env_keys` list, so a
  member can be granted app access for a subset of environments.

Add `requireEnvironmentKey` next to the other environment crypto helpers
and use it at each site, so the failure names the environment and says
what to do about it. Behaviour is otherwise unchanged: the sites that
threw still throw, with a message that means something.
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.

1 participant