Skip to content

Commit 8747618

Browse files
authored
Record the series key by id in audit metadata, not by name (#108)
CodeQL flagged the Series API's audit write as clear-text storage of sensitive data (rb/clear-text-storage-sensitive-data, high): it traces the read of `name` off a series API token into audit_logs.metadata, which is a plain jsonb column. The name was never a secret -- it is the display label built from the creator's email local part, and the secret itself is only ever held as a SHA256 digest. But the metadata does not need the label: storing the key's id instead removes the flagged dataflow entirely and is more useful to an auditor, since an id survives a rename or a rotation, resolves to the key's name, creator and revocation state, and revoking a key keeps its row.
1 parent 583c411 commit 8747618

2 files changed

Lines changed: 7 additions & 3 deletions

File tree

app/controllers/api/v1/series/events_controller.rb

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,7 @@ def grant_token_owner_event_admin
160160
end
161161

162162
# API-key requests have no current_user, so the token's owner is the
163-
# closest thing to an actor; the token name goes in the metadata either
163+
# closest thing to an actor; the key's id goes in the metadata either
164164
# way so a series key's writes are attributable.
165165
def log_event_change(action)
166166
AuditLog.log!(
@@ -174,7 +174,11 @@ def log_event_change(action)
174174
user_agent: request.user_agent,
175175
source: "series_api",
176176
series_id: @series.id,
177-
series_api_token_name: current_series_api_token&.name
177+
# The key's id, not its name: audit_logs.metadata is stored in
178+
# clear text, and an id is provably not a credential. It also
179+
# outlives a rename or a rotation, and revoking a key keeps its
180+
# row, so this always resolves to the key that acted.
181+
series_api_token_id: current_series_api_token&.id
178182
}.compact
179183
)
180184
rescue StandardError => e

spec/requests/api/v1/series_events_spec.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ def valid_attributes(overrides = {})
154154
expect(log.action).to eq("record_create")
155155
expect(log.actor).to eq(owner)
156156
expect(log.metadata["source"]).to eq("series_api")
157-
expect(log.metadata["series_api_token_name"]).to eq("owner-series-events@ops")
157+
expect(log.metadata["series_api_token_id"]).to eq(SeriesApiToken.last.id)
158158
end
159159

160160
describe "non-series credentials" do

0 commit comments

Comments
 (0)