feat(meetings): connect a calendar's credentials and OAuth from Settings - #8081
feat(meetings): connect a calendar's credentials and OAuth from Settings#8081kaizawa97 wants to merge 2 commits into
Conversation
github-actions
Bot
added
fork
7af15db to
4530692
Compare
github-actions
Bot
added
readiness: checking
github-actions
Bot
added
the
merge conflict
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
NicholasRBowers
added
needs-author-decision
|
🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]: This PR has been inactive for 7+ days. I reviewed the blockers but they require your input:
When you've addressed these, the pipeline will re-assess on its next cycle. |
…osoft 365
Meetings had to be created by hand, so the app never knew what was already
scheduled. This adds calendar providers: CalDAV, Google Calendar and
Microsoft 365, behind one interface, plus the credential store and OAuth
handshake they need.
Security decisions worth naming, because they shaped the code:
Credentials live under `<crew-home>/workspace/meetings/`, not
`app_data_dir("meetings")`. The latter is a region `store.contain` opens to
agent-supplied paths, and a calendar refresh token is not something to keep
where an agent can ask for a file.
The address vet is `link_unfurl.vet_unfurl_url`, reused rather than
reimplemented. This started as a purpose-built check in `calendar.py` and that
was a mistake: a second copy of this logic is a second place to fix a bypass,
and the copy was already weaker in four ways the shared one covers.
0177.0.0.1 approved, then fetched at 177.0.0.1 — `ipaddress` will
not read the octal, so it fell through to DNS and
getaddrinfo read `0177` as decimal. The vet and the
connection disagreed about the target, which is the exact
class of bug the pinning exists to prevent.
100.64.0.0/10 approved. `is_private` does not cover CGNAT; only
`is_global` does. On a machine on a tailnet, that range
IS the private network.
fec0::/10 approved. Deprecated IPv6 site-local reports is_global.
.local / .onion approved. Resolve through a side channel, or not at all.
`link_unfurl` also owns `test_vet_rejects_every_special_purpose_range`, which
pins the refusal set against a table of IANA special-purpose prefixes — so the
next gap is found by the suite instead of by a reviewer. A second
implementation here would not have inherited that.
One refusal went the other way and is kept, layered on top: a resolved address
that `ipaddress` cannot read is refused, not skipped.
`_reject_if_internal_ip` returns silently for a non-literal, because to it a
non-literal is a hostname still to be resolved; here the list is already a
resolution result, so an unreadable entry would reach the pin unchecked.
The pin, the redirect hop loop and the same-origin check stay local. They are
not duplicated — `VettedUrl`'s own docstring says to pin the resolver on
`wire_host`, so this is the caller side of that contract. `resolve` is injected
only to keep every address the vet approved, since `VettedUrl` reports one and
a multi-homed calendar host should keep its fallbacks.
Ports narrow to 80/443, which https-only leaves as 443. That is stricter than
"whatever port the URL names", deliberately: a calendar on another port is
nearly always an internal service, and the port is the cheapest place to stop
this endpoint being used to probe for one. Nothing is broken by starting
strict — this feature has never shipped — and relaxing it later is one line.
XML parsing rejects a DOCTYPE outright. Measured, not assumed: the stdlib
ElementTree expands internal entities while refusing external ones, and
`XMLParser.doctype` is ignored on 3.12, so the rejection is done in
`TreeBuilder.doctype` where it is actually reached. `defusedxml` would be the
obvious answer but `test_pip_deps_consistency.py` pins the extras, so adding
a dependency is a wider change than this needs.
Redaction is applied once, in `build_event()`, rather than per provider — a
new provider then cannot forget it, and `security_posture.py` gains one sink
instead of three.
The OAuth redirect URI is derived from the request's own origin. Dashboard
auth is an HMAC-signed cookie scoped to the host, and the port is
configurable, so a constant would break on any non-default port and
`localhost` vs `127.0.0.1` would not share the cookie.
Drive-to-green amendments by Kiro Crew (original work by kaizawa97):
rebased onto current main; SEL-audit the invalid/expired/forged-state
rejection in the OAuth callback so the anti-forgery refusal is visible
in the audit trail, with a test locking the record in.
Coverage Gate: added route-level tests for the credential GET/PUT/forget
surface, the OAuth start route, the callback success path, and the
credential store's failure-path contracts (atomic-write cleanup,
owner-lockdown degradation, empty-provider refusal), lifting
routes/calendar.py 56%->95% and credentials.py 77%->100% past the 80%
per-file floor without touching the baseline.
Review round 2: the credential store now writes through the repo's
canonical atomic_write helper (restrict_to_owner=True, fail-closed) —
the temp is locked to the owner BEFORE content lands and a lockdown
failure refuses the write instead of publishing tokens under an
inherited Windows ACL, closing a GPT blocking finding and deleting the
hand-rolled temp+fsync+replace copy First Principles flagged.
Review round 3: event ids are derived from the ORIGINAL provider uid,
not the redacted form. Microsoft Graph ids are long base64 blobs that
trip the credential redactor's entropy heuristic, so different events
collapsed to one [REDACTED] placeholder, one shared digest, one shared
meeting directory. Redaction still applies to every displayed field;
_event_id_for makes the raw uid filesystem-safe itself.
Review round 4: (1) schema-invalid credential stores now raise
_StoreUnreadable on every invalid root/entry/value instead of silently
dropping entries — a partial view must never be rewritten back, the
same contract the parse-failure path already had. (2) A uid the
credential redactor flags switches to a digest-only event-id source, so
a credential-shaped uid never surfaces a readable stem in the
agent-visible id (while staying unique per uid and stable across
syncs). (3) https-only now implies port 443 at the vet boundary, keyed
on the scheme allow-list so the real-server rebinding tests keep
working.
Review round 5: hoisted the oauth import in routes/calendar.py to
module scope (no circular dependency exists; oauth imports
providers.calendar, nothing imports back into routes). The symlink
finding on the credential directory is rebutted on the PR with
reachability evidence; class-level parent-symlink refusal belongs in
the shared atomic_write helper and is tracked as a follow-up issue.
Rebased onto main to absorb the new baselined black gate; ran
black --target-version py310 on the three in-scope offenders.
Rebased onto current main (1600+ commits), reconciling with the meetings
fixes that landed there since this branch's last rebase:
* kirodotdev#4380 (all-day rendering): the all_day flag is threaded through this
PR's build_event funnel so .ics all-day events keep main's date-only
semantics; providers still default to timed events.
* kirodotdev#5557 (Windows TZID -> IANA): _graph_when now consults the same
_WINDOWS_TO_IANA table before falling back to UTC, closing the
"Tokyo Standard Time read as UTC" gap for Graph responses that ignore
the Prefer header. Regression tests updated to pin the mapping and the
unknown-zone fallback separately.
* The workspace/meetings credential-store entry made the variable-leaf
gate fence the whole crew workspace (breaking main's pinned
`cat ~/.kiro/crew/workspace/$PROJ/notes.md`); the workspace parent is
now in _GENERAL_PURPOSE_PARENT_DIRS per the .config/gcloud precedent,
with the meetings child still fully fenced by is_sensitive_path.
The setup.cfg defusedxml hunk is dropped: main now declares defusedxml
in install_requires itself, so the hunk had become a duplicate
declaration with a conflicting ceiling.
Co-authored-by: Kiro Crew <noreply@kirodotdev.github.io>
The calendar providers that need a credential (CalDAV, Google Calendar, Microsoft 365) could only be connected by calling the credentials and OAuth routes by hand: Settings -> Calendar showed a provider picker and a source field and nothing else. Settings -> Calendar now renders the credential form for the active provider under the picker. The form's shape comes from the backend: GET /calendar/credentials gains a `providers` map built from the same allowlist and OAuth table the PUT enforces, so the fields shown are the fields that can be written and a provider that takes none renders nothing. Every field is write-only (a stored value never reaches the browser): a set field shows a mask with Replace / Remove, a typed value is sent as a string, a removed one as null, an untouched one not at all. "Connected" is derived from field names -- a refresh token for OAuth, every field for a password provider -- and re-reads on window focus, because the OAuth consent finishes in another tab. Sign-in stays disabled until a client id is stored; it opens the consent URL in a new tab and offers the same URL as a link when a popup blocker refuses. Disconnect drops the provider's credentials and any pending flow. Strings land in all twelve locales; the pseudolocale is regenerated.
4530692 to
c53d1a2
Compare
|
Rebased onto main
Main's new Ran locally: black/isort/flake8 on changed files, the 4 meetings test files (509 passed), all Please review the resolution. A maintainer push makes the maintainer the last pusher, so a second approver is needed under the repo's last-push rule. Reply if anything looks wrong. |
github-actions
Bot
added
readiness: checking
First Principles Review (Fable 5, fork) — 🟡 CONCERNSPremise-level review of All hunks check out: no deleted pins, this PR's own items each have a named harm, the core First-Principles-Verdict: CONCERNS The OAuth handshake ships never run against a real tenant; "Google and Microsoft accept a loopback redirect on any port" is an untested provider claim. Not justified as shipped
What this change shipsIntent: let a user connect CalDAV / Google / Microsoft 365 calendars from Settings instead of hand-crafting curl requests — an ADDITION.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] c53d1a2 |
UX Review (Fable 5, fork) — 🟡 CONCERNSUX-level review of I have everything I need: the full frontend diff, the English strings, the callback HTML, and the sibling Connections surface for comparison. The fork's screenshots are not materialized in this checkout, so no first-time reader has seen any of the new controls. UX-Verdict: CONCERNS Solid write-only credential flow, but "Connected" is claimed unverified, the popup-blocked toast describes a tab that never opened, and no cold reader has seen any of it. Watch
Evidence gaps
Suggestions
[UX-REVIEWED] c53d1a2 |
Design Review (Fable 5, fork) — 🔴 BLOCK (blocking)Design-level review of Design-Verdict: BLOCK The OAuth callback sits behind token auth with no bypass, so the consent leg the PR never tested cannot complete from the Electron shell. BlockersOAuth sign-in cannot complete on the desktop shell (and this is exactly the leg manual verification skipped) WatchFive binary screenshots are committed under Suggestions
[DESIGN-REVIEWED] c53d1a2 |
Opus 4.8 Review (fork) — 🔴 changes requested (blocking)Reviewed Blocks: two [BLOCK-MERGE] c53d1a2 BLOCKING — website/src/apps/meetings/components/CalendarCredentials.tsx:153 BLOCKING — website/src/apps/meetings/components/CalendarCredentials.tsx:171 FINDING — website/src/apps/meetings/components/CalendarCredentials.tsx:120 — [OPUS-REVIEWED] c53d1a2 |
GPT 5.6 Review (fork) — 🔴 changes requested (blocking)Reviewed 2 of 4 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands. BLOCKING -- website/src/apps/meetings/components/CalendarCredentials.tsx:92 -- Credential failures lack a persistent ErrorNotice BLOCKING -- website/src/apps/meetings/components/CalendarCredentials.tsx:171 -- OAuth credentials render three sibling buttons BLOCKING -- src/kiro_crew/apps/builtins/meetings/backend/credentials.py:284 -- Disconnect can be undone by an in-flight token refresh (origin: validation) BLOCKING -- src/kiro_crew/apps/builtins/meetings/backend/providers/calendar.py:1071 -- Cross-origin redirects resend OAuth secrets (origin: validation) FINDING -- src/kiro_crew/apps/builtins/meetings/backend/credentials.py:183 -- FINDING -- src/kiro_crew/apps/builtins/meetings/backend/providers/calendar.py:10 -- [BLOCK-MERGE] c53d1a2 Adjudication (Opus 4.8) — is blocking on each finding proportionate?I've verified all four findings against the code. Both adjudicable findings anchor to AUTOSDE rules carrying F1 — F2 — F3 — Real read-before-delete / write-after race. F4 — Body (carrying code, verifier, refresh_token, client_secret) is resent on every hop including cross-origin 307/308 ( [ADJUDICATION] c53d1a2 total=2 uphold=2 downgrade=0 🏷️ Fenced finding(s) machine-flagged as likely edge caseThe security fence keeps these findings blocking regardless of adjudication; the only clearance path is a human override recorded by a repository writer, who must independently verify a rationale before recording it — it is machine-authored, and a wrong override on a security-class finding ships exactly the class the fence exists to stop. (This lane's comment deliberately carries no override command.)
|
github-actions
Bot
added
readiness: action required
4 participants
Problem / Motivation
#2190 adds the CalDAV, Google Calendar and Microsoft 365 providers and the
routes to store their credentials and run the OAuth handshake, but no screen
calls them. Settings → Calendar shows a provider picker and a source field and
nothing else, so the three new providers can only be connected by hand-crafting
PUT /calendar/credentialsandPOST /calendar/oauth/startrequests.Why it matters
A provider nobody can configure from the product is not shipped. Until this
lands, the calendar picker offers three choices that lead nowhere for anyone
who is not comfortable with curl, and the security work in #2190 (write-only
store, OAuth with PKCE) protects credentials that no user can enter.
What changed (motivation → approach → change)
Goal. Let a user connect CalDAV, Google or Microsoft 365 from Settings
without the page ever seeing a stored value and without a second, hand-kept
list of what each provider needs.
Approach. The form's shape comes from the backend, not the frontend. #2190's
write path already owns two tables — the per-provider field allowlist
(
_CREDENTIAL_FIELDS) and the OAuth client map (_OAUTH_CLIENTS). Exposing themin the GET is what keeps the fields a user sees and the fields a PUT accepts from
ever disagreeing, and keeps an out-of-repo provider able to describe its own
form. Rendering reuses the dashboard's existing write-only
SecretField, so astored secret gets the same mask / Replace / Remove treatment as a Slack token.
Change.
GET /calendar/credentialsgainsproviders, a{provider: {fields, oauth}}map derived from those two tables. Pinned by atest that asserts the map equals the allowlist and omits
ics.website/src/apps/meetings/components/CalendarCredentials.tsx(new). Under theprovider picker, for the active provider: a status badge (Connected /
Credentials saved / Not connected), one
SecretFieldper field, Savecredentials, Sign in with {provider} for OAuth providers, Disconnect
once anything is stored. Renders nothing for a provider the schema does not
list (
none,ics).null, anuntouched field is not sent at all — so "leave it alone to keep it" holds
without the page ever knowing the value.
refresh_tokenfor OAuth, everylisted field for a password provider. The status query has
staleTime: 0and refetches on window focus because the consent finishes in another tab.
client_idis stored. It POSTsoauth/startand opens the consent URL withwindow.open(url, '_blank', 'noopener,noreferrer')— the Electron shellforwards that to the OS browser — and, when a popup blocker answers
null,offers the same URL as a link.
SettingsView.tsxmounts the component inside the Calendar card;api.tsgains the four client methods and the wire types.
apps.meetings.settings.*in all 12 locales, translatorcontext for the five ambiguous ones, pseudolocale regenerated.
docs/system-specs/modules/meetings.md: the five feat(meetings): read the user's calendar over CalDAV, Google and Microsoft 365 #2190 routes are added tothe route table (they were missing), plus a "Credentials in Settings" section
and the new component in the layout and tests lists.
Tests
website/src/test/MeetingsCalendarCredentials.test.tsx(13): renders nothingfor a schema-less provider; renders the backend's fields and Not connected;
saves only what was typed and never echoes a value into the DOM; sends
nullfor a removed field; keeps the draft on a failed save; disconnects through the
forget route; keeps sign-in disabled without a client id; opens the consent URL
via
window.openwithnoopener,noreferrer; falls back to a link when thepopup is blocked; reports a refused sign-in without opening anything; shows
Connected once a refresh token is stored; renders nothing when the status
cannot be read.
isConnectedis unit-tested on its own.MeetingsSettingsViewCoverage.test.tsxgains a resolved credentials mock.test_get_describes_each_providers_form_from_the_allowlistintest_meetings_calendar_routes.py.Component coverage 96.9%. Local:
tsc -b, eslint, vitest (110 across the fourtouched suites incl. catalog parity and dead keys),
npm run i18n:check(allrows ok), focus-cue and phantom-class gates, 47 backend route tests, flake8,
baselined black, docs-lint.
Manual verification
Screenshots below were taken against a gateway built from this branch with an
isolated
KIROCREW_HOME, driving the real routes: CalDAV credentials savedthrough the UI flip the badge to Connected and mask both fields; a Google client
id saved through the UI enables Sign in. The OAuth consent itself was not
completed against a live Google or Microsoft tenant (no registered client here);
the handshake is covered by #2190's tests, and this PR asserts its side at the
window.openboundary.Screenshots / video
CalDAV, nothing stored:
CalDAV after saving a username and password — masked, Connected, Disconnect
available:
Google Calendar with a client id stored — sign-in enabled:
More
Google before a client id is stored (sign-in disabled, hint shown):
Full settings page:
Related Issues
Depends on #2190. The poller PR (
pr/meetings-calendar-poller) is independent.Checklist
meetings.md)