Skip to content

Expose organization details and email updates - #184

Open
doobneek wants to merge 5 commits into
developfrom
fix/organization-email-access-control
Open

Expose organization details and email updates#184
doobneek wants to merge 5 commits into
developfrom
fix/organization-email-access-control

Conversation

@doobneek

@doobneek doobneek commented Mar 9, 2026

Copy link
Copy Markdown

Supersedes #176.

Summary

  • require authentication for direct organization reads
  • validate and normalize organization email on create and update
  • exclude organization email from public organization and location payloads
  • add regression tests for email validation and public/private organization serialization

@doobneek doobneek closed this Mar 9, 2026
@doobneek doobneek reopened this Mar 9, 2026
@doobneek
doobneek temporarily deployed to CI_CD_PIPELINE March 9, 2026 22:43 — with GitHub Actions Inactive
@doobneek
doobneek temporarily deployed to CI_CD_PIPELINE March 9, 2026 22:43 — with GitHub Actions Inactive

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cannot safely approve due to a privacy/access-control regression: organization email can be fetched without authorization via the new organization detail endpoint.

Blocking issues

  • Privacy/access-control issue: GET /organizations/:organizationId is publicly reachable (only getUser middleware) and returns full Organization via findByPk, including email (src/routes.js, src/controllers/organizations.js). The new test explicitly validates unauthenticated email exposure (test/integration/get-organizations.test.js). This is a blocking PII exposure risk.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR correctly removes organization emails from public list/location payloads, but it introduces a new endpoint that appears to re-expose email without clear authorization guarantees.

Blocking issues

  • Potential privacy/access-control regression: GET /organizations/:organizationId (added in src/routes.js and implemented in src/controllers/organizations.js) returns the full organization record including email, but it is not protected by dataEntryAuth or any organization-level authorization check. If getUser is optional/permissive in this codebase, this enables email harvesting by ID enumeration.
  • Missing risky auth tests: there are no integration tests asserting unauthenticated/unauthorized behavior for GET /organizations/:organizationId (e.g., 401/403 expectations). Given this endpoint returns previously-hidden PII (email), auth behavior must be explicitly tested before approval.

@doobneek
doobneek temporarily deployed to CI_CD_PIPELINE March 9, 2026 22:53 — with GitHub Actions Inactive

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not safe to approve as-is due to an access-control gap introduced around organization email updates.

Blocking issues

  • High severity (auth/integrity): PATCH /organizations/:organizationId now allows updating email (src/controllers/organizations.js), but the route still has no organization-level authorization check (src/routes.js uses only getUser + dataEntryAuth). In production, any authenticated user can modify any organization’s private contact email.
  • Risky logic lacks protective tests: new tests cover email format/trim behavior, but there is no integration test asserting unauthorized users cannot update another organization’s email (or organization record) via PATCH /organizations/:organizationId.

@doobneek

Copy link
Copy Markdown
Author

Addressed all bot comments in the current head commit.

  • direct organization reads now require org-member/admin authorization in the controller
  • organization updates now require the same org-member/admin authorization
  • integration tests now cover 401/403/200 cases for both GET and PATCH
  • email validation/normalization and public payload redaction remain in place

@doobneek
doobneek dismissed stale reviews from github-actions[bot] and github-actions[bot] March 10, 2026 01:21

Addressed in commits 6016f86 and a93fd8f: organization detail and update endpoints now enforce org-member/admin authorization, and GET/PATCH auth regression tests were added.

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