feat: wire M365 broker into auth server#47
Conversation
|
Warning Review limit reached
More reviews will be available in 25 minutes and 56 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis PR adds a complete Microsoft 365 OAuth 2.0 broker to the auth-server, including PKCE-backed session management, email verification via Microsoft Graph, configuration resolution from environment variables, and CLI integration. The auth-server can now operate in M365-only or combined Google/M365 mode. ChangesMicrosoft 365 OAuth Broker Integration
Sequence DiagramssequenceDiagram
participant Client
participant Server
participant Graph as Microsoft Graph
participant Tokens as Token Store
Client->>Server: POST /m365/sessions (expected_email)
Server->>Server: validate M365 config<br/>generate PKCE session
Server-->>Client: 200 OK (state, login_url)
Client->>Server: GET /m365/start/{state}
Server->>Server: fetch session
Server-->>Client: 302 redirect to authorization
Client->>Server: GET /m365/callback (code, state)
Server->>Server: exchange code + PKCE<br/>for tokens
Server->>Graph: GET /me (access_token)
Graph-->>Server: email
Server->>Server: validate email matches
Server->>Tokens: store token
Server-->>Client: 200 OK success page
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Microsoft 365 (M365) integration to the auth server, adding endpoints for one-click login sessions, PKCE-based OAuth flow, and profile validation. It also updates the Dockerfile to use a secure, multi-stage distroless build and refactors the CLI command to dynamically resolve broker URLs. Feedback on these changes highlights three key areas for improvement: implementing a passive cleanup mechanism in m365SessionStore to prevent memory leaks from expired sessions, limiting the request body size in the /m365/sessions endpoint using http.MaxBytesReader to mitigate Denial of Service risks, and simplifying the URL resolution logic in the CLI command to avoid potential bugs when only a custom base URL is provided.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func (s *m365SessionStore) save(session m365Session) { | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
|
|
||
| if s.sessions == nil { | ||
| s.sessions = make(map[string]m365Session) | ||
| } | ||
|
|
||
| s.sessions[session.State] = session | ||
| } |
There was a problem hiding this comment.
The m365SessionStore does not have a background cleanup routine for expired sessions. If sessions are created but never started or consumed, they will remain in the sessions map indefinitely, leading to a memory leak. We can implement a passive cleanup of expired sessions inside the save method to prevent unbounded memory growth without needing extra background goroutines.
func (s *m365SessionStore) save(session m365Session) {
s.mu.Lock()
defer s.mu.Unlock()
if s.sessions == nil {
s.sessions = make(map[string]m365Session)
}
// Passive cleanup of expired sessions to prevent memory leaks
now := time.Now()
for k, sess := range s.sessions {
if !sess.ExpiresAt.IsZero() && now.After(sess.ExpiresAt) {
delete(s.sessions, k)
}
}
s.sessions[session.State] = session
}| var req struct { | ||
| ExpectedEmail string `json:"expected_email"` | ||
| ForceConsent bool `json:"force_consent"` | ||
| } | ||
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | ||
| writeJSONError(w, http.StatusBadRequest, err) | ||
| return | ||
| } |
There was a problem hiding this comment.
The request body is decoded without limiting its size. An attacker could send an extremely large payload to POST /m365/sessions, causing high memory consumption and potential denial of service. It is recommended to wrap the request body in an http.MaxBytesReader to enforce a reasonable limit (e.g., 1MB).
| var req struct { | |
| ExpectedEmail string `json:"expected_email"` | |
| ForceConsent bool `json:"force_consent"` | |
| } | |
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | |
| writeJSONError(w, http.StatusBadRequest, err) | |
| return | |
| } | |
| r.Body = http.MaxBytesReader(w, r.Body, 1048576) // Limit to 1MB | |
| var req struct { | |
| ExpectedEmail string `json:"expected_email"` | |
| ForceConsent bool `json:"force_consent"` | |
| } | |
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | |
| writeJSONError(w, http.StatusBadRequest, err) | |
| return | |
| } |
| func (c *AuthM365LoginLinkCmd) resolveBrokerURLs() (string, string, error) { | ||
| baseURL := strings.TrimSpace(c.BaseURL) | ||
| callbackURL := strings.TrimSpace(c.CallbackURL) | ||
| if baseURL == "" || callbackURL == "" { | ||
| callbackServer, err := googleauth.CallbackServerURL(baseURL) | ||
| if err != nil { | ||
| return "", "", err | ||
| } | ||
|
|
||
| if baseURL == "" { | ||
| baseURL = strings.TrimRight(callbackServer, "/") | ||
| } | ||
| if callbackURL == "" { | ||
| callbackURL = strings.TrimRight(callbackServer, "/") + "/m365/callback" | ||
| } | ||
| } |
There was a problem hiding this comment.
The current URL resolution logic is overly complex and potentially buggy. If a user specifies a custom --base-url but omits --callback-url, the callbackURL is derived from googleauth.CallbackServerURL(baseURL) (which might return the default callback server instead of the custom base URL). We can simplify this logic to resolve baseURL first if empty, and then derive callbackURL directly from baseURL if it is empty.
func (c *AuthM365LoginLinkCmd) resolveBrokerURLs() (string, string, error) {
baseURL := strings.TrimSpace(c.BaseURL)
callbackURL := strings.TrimSpace(c.CallbackURL)
if baseURL == "" {
callbackServer, err := googleauth.CallbackServerURL("")
if err != nil {
return "", "", err
}
baseURL = strings.TrimRight(callbackServer, "/")
}
if callbackURL == "" {
callbackURL = baseURL + "/m365/callback"
}![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c77c9f1ea9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return s | ||
| } | ||
|
|
||
| func (s *Server) handleM365Sessions(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
When the broker is reachable at the configured public base URL, this unauthenticated POST lets any caller create a Microsoft 365 state for any expected email; the response exposes that state/login URL, and after the target completes consent the existing unauthenticated /token/{state} handler returns the refresh token. That makes it possible to generate a link for a victim and then consume their token, so session creation needs to be restricted to a trusted/admin caller or otherwise protected before exposing this endpoint.
Useful? React with 👍 / 👎.
| session, err := createM365BrokerSession(ctx, msauth.BrokerSessionOptions{ | ||
| ExpectedEmail: email, | ||
| BaseURL: c.BaseURL, | ||
| CallbackURL: c.CallbackURL, | ||
| BaseURL: baseURL, | ||
| CallbackURL: callbackURL, | ||
| Readonly: true, | ||
| ForceConsent: c.ForceConsent, | ||
| TTL: c.TTL, |
There was a problem hiding this comment.
When users run auth m365 login-link, this still creates the state only in the local CLI process via msauth.CreateBrokerSession; the auth server's /m365/start/{state} path only checks its in-memory s.m365Sessions, which is populated by POST /m365/sessions. As a result, the printed one-click URL points at the broker with a state the broker has never seen and returns 404 instead of redirecting to Microsoft; the command needs to create the session on the broker (or otherwise store it server-side) before returning the link.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
auth-server/README.md (1)
104-108:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCompose healthcheck uses
wget, which doesn't exist in the new distroless image.The Dockerfile now builds on
gcr.io/distroless/static-debian12:nonroot, which has no shell orwget. This Compose example builds that same image (build: .) but its healthcheck shells out towget, so the container will be reported unhealthy. Consider removing the healthcheck here or switching to an in-binary health probe.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@auth-server/README.md` around lines 104 - 108, The Compose healthcheck in README.md uses wget which is not present in the distroless image; remove or replace the healthcheck so it doesn't shell out to wget. Either remove the entire healthcheck block from the Compose example, or replace it with an in-binary probe that calls your service binary directly (e.g., change the test to call your application's health probe executable), and update the README to note that a distroless runtime requires in-binary or container-local probes instead of shell utilities.
🧹 Nitpick comments (5)
auth-server/main.go (1)
83-86: 💤 Low valueDead code: redirect URL default is now unreachable.
resolveServerConfigalways returns a non-emptyredirectURL(it falls back tohttp://localhost:<port>/callbackinconfig.go), so after Line 51*redirectURLcan never be empty. This block can be removed.♻️ Proposed cleanup
- // Default redirect URL if not specified - if *redirectURL == "" { - *redirectURL = fmt.Sprintf("http://localhost:%d/callback", *port) - } - // Create token store with TTL and start cleanup🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@auth-server/main.go` around lines 83 - 86, The if-block that sets a default for *redirectURL is dead code because resolveServerConfig always returns a non-empty redirectURL (it already falls back to "http://localhost:<port>/callback" in config.go); remove the entire block starting with "if *redirectURL == "" { ... }" in main.go (references: redirectURL variable and resolveServerConfig function) to clean up unreachable logic.auth-server/Dockerfile (1)
10-10: 💤 Low valueHardcoded
GOARCH=amd64produces an amd64-only image.If the deployment target (OCI/HV pod) can be arm64, this build will produce an incompatible binary. Consider letting Docker's
TARGETARCHbuild arg drive the architecture so multi-arch builds work.♻️ Use BuildKit TARGETARCH
-FROM golang:1.25-alpine AS build +FROM --platform=$BUILDPLATFORM golang:1.25-alpine AS build +ARG TARGETARCH WORKDIR /src COPY go.mod go.sum ./ RUN go mod download COPY . ./ -RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -trimpath -ldflags="-s -w" -o /out/workit-auth-server . +RUN CGO_ENABLED=0 GOOS=linux GOARCH=${TARGETARCH} go build -trimpath -ldflags="-s -w" -o /out/workit-auth-server .🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@auth-server/Dockerfile` at line 10, Replace the hardcoded GOARCH=amd64 in the Dockerfile RUN that builds the Go binary with a build-arg driven value so multi-arch images work: add an ARG TARGETARCH (or ensure existing ARG is present) and use GOARCH=${TARGETARCH} in the build invocation (the RUN line that calls "CGO_ENABLED=0 GOOS=linux GOARCH=... go build -trimpath -ldflags=\"-s -w\" -o /out/workit-auth-server .") so the architecture comes from Docker BuildKit's TARGETARCH during multi-platform builds.auth-server/m365.go (3)
52-107: ⚖️ Poor tradeoffExpired sessions are never reclaimed unless accessed.
getevicts only on access andconsumedeletes on use, but a session that is created and then never started/consumed remains in the map until process restart. Over time (or under abuse ofPOST /m365/sessions) this is an unbounded-growth/memory pressure risk. Consider a background sweep (e.g., atime.Tickergoroutine pruning entries pastExpiresAt) or pruning lazily on eachsave.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@auth-server/m365.go` around lines 52 - 107, m365SessionStore currently can grow unbounded because expired sessions are only removed on get/consume; add eviction to prevent memory growth by either (A) starting a background goroutine in newM365SessionStore that runs a time.Ticker and prunes entries whose m365Session.ExpiresAt is set and before time.Now(), locking s.mu while deleting, or (B) augment save(session m365Session) to scan and remove expired entries before inserting; reference the m365SessionStore type and its methods save, get, consume and the ExpiresAt field when implementing the sweep so expired sessions are reclaimed without requiring explicit access.
180-185: 💤 Low valueRender an HTML error page instead of JSON for browser-facing start endpoint.
/m365/start/{state}is hit directly by a user's browser (the generatedlogin_url). On an expired/unknown state this returns a raw JSON error viawriteJSONError, which is a confusing UX. The callback path already usesrenderErrorPage; reuse it here for consistency.♻️ Proposed change
session, err := s.m365Sessions.get(state) if err != nil { - writeJSONError(w, http.StatusNotFound, err) + s.renderErrorPage(w, "Microsoft 365 login link expired or not found", http.StatusNotFound) return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@auth-server/m365.go` around lines 180 - 185, When an unknown/expired state is detected after calling s.m365Sessions.get(state) replace the browser-facing JSON response with the same HTML error flow used by the callback: call renderErrorPage(w, r, http.StatusNotFound, err.Error() or a user-friendly message) instead of writeJSONError, so the /m365/start/{state} endpoint renders the HTML error page; keep the HTTP status consistent (http.StatusNotFound) and reuse the same renderErrorPage signature used elsewhere to maintain UX consistency.
281-290: 💤 Low valueConsider pinning
AuthStyleInParamsfor the Microsoft token endpoint.This is a public client (no client secret). With the default
AuthStyleAutoDetect,oauth2first attempts HTTP Basic and only falls back to in-params on failure, producing an extra failed token request against Microsoft on first exchange per client style cache. Microsoft's v2.0 token endpoint expects credentials in the body; settingAuthStyle: oauth2.AuthStyleInParamsavoids the wasted round-trip and the brittle auto-detect path.golang.org/x/oauth2 AuthStyleAutoDetect behavior with empty client secret and Microsoft identity v2.0 token endpoint🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@auth-server/m365.go` around lines 281 - 290, The oauth2.Config returned by Server.m365OAuthConfig uses the default AuthStyleAutoDetect which causes an unnecessary failed HTTP Basic attempt against Microsoft's v2 token endpoint; update m365OAuthConfig to set AuthStyle: oauth2.AuthStyleInParams on the returned oauth2.Config so the public client sends credentials in the request body (in-params) and avoids the extra failed token request.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@auth-server/m365.go`:
- Around line 380-388: validateM365Email currently returns an error that embeds
user emails (want/got) which leaks PII; change validateM365Email to avoid
including the email values in the returned error — either return the sentinel
errM365EmailMismatch directly or wrap it with non-PII context (e.g.,
fmt.Errorf("%w: email mismatch") ) and leave actual values out so the caller
(log.Printf in the M365 flow) will not log user emails; update the function
validateM365Email to use only errM365EmailMismatch (or a non-PII wrapper)
instead of formatting want/got into the error.
In `@internal/cmd/auth_m365_link.go`:
- Around line 47-52: The callbackURL is not validated — parse and validate it
the same way as baseURL: call url.Parse(callbackURL) and ensure the resulting
URL has a non-empty Scheme and Host, returning usage("invalid m365 broker
--callback-url") on error; update the function that currently parses baseURL
(references: url.Parse, baseURL, callbackURL) so both URLs are validated before
returning and before calling createM365BrokerSession.
---
Outside diff comments:
In `@auth-server/README.md`:
- Around line 104-108: The Compose healthcheck in README.md uses wget which is
not present in the distroless image; remove or replace the healthcheck so it
doesn't shell out to wget. Either remove the entire healthcheck block from the
Compose example, or replace it with an in-binary probe that calls your service
binary directly (e.g., change the test to call your application's health probe
executable), and update the README to note that a distroless runtime requires
in-binary or container-local probes instead of shell utilities.
---
Nitpick comments:
In `@auth-server/Dockerfile`:
- Line 10: Replace the hardcoded GOARCH=amd64 in the Dockerfile RUN that builds
the Go binary with a build-arg driven value so multi-arch images work: add an
ARG TARGETARCH (or ensure existing ARG is present) and use GOARCH=${TARGETARCH}
in the build invocation (the RUN line that calls "CGO_ENABLED=0 GOOS=linux
GOARCH=... go build -trimpath -ldflags=\"-s -w\" -o /out/workit-auth-server .")
so the architecture comes from Docker BuildKit's TARGETARCH during
multi-platform builds.
In `@auth-server/m365.go`:
- Around line 52-107: m365SessionStore currently can grow unbounded because
expired sessions are only removed on get/consume; add eviction to prevent memory
growth by either (A) starting a background goroutine in newM365SessionStore that
runs a time.Ticker and prunes entries whose m365Session.ExpiresAt is set and
before time.Now(), locking s.mu while deleting, or (B) augment save(session
m365Session) to scan and remove expired entries before inserting; reference the
m365SessionStore type and its methods save, get, consume and the ExpiresAt field
when implementing the sweep so expired sessions are reclaimed without requiring
explicit access.
- Around line 180-185: When an unknown/expired state is detected after calling
s.m365Sessions.get(state) replace the browser-facing JSON response with the same
HTML error flow used by the callback: call renderErrorPage(w, r,
http.StatusNotFound, err.Error() or a user-friendly message) instead of
writeJSONError, so the /m365/start/{state} endpoint renders the HTML error page;
keep the HTTP status consistent (http.StatusNotFound) and reuse the same
renderErrorPage signature used elsewhere to maintain UX consistency.
- Around line 281-290: The oauth2.Config returned by Server.m365OAuthConfig uses
the default AuthStyleAutoDetect which causes an unnecessary failed HTTP Basic
attempt against Microsoft's v2 token endpoint; update m365OAuthConfig to set
AuthStyle: oauth2.AuthStyleInParams on the returned oauth2.Config so the public
client sends credentials in the request body (in-params) and avoids the extra
failed token request.
In `@auth-server/main.go`:
- Around line 83-86: The if-block that sets a default for *redirectURL is dead
code because resolveServerConfig always returns a non-empty redirectURL (it
already falls back to "http://localhost:<port>/callback" in config.go); remove
the entire block starting with "if *redirectURL == "" { ... }" in main.go
(references: redirectURL variable and resolveServerConfig function) to clean up
unreachable logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4105744-8245-48ac-8cdb-684a3de4ca0f
📒 Files selected for processing (12)
Makefileauth-server/Dockerfileauth-server/README.mdauth-server/config.goauth-server/dockerfile_test.goauth-server/handlers.goauth-server/m365.goauth-server/m365_handlers_test.goauth-server/main.goauth-server/main_config_test.gointernal/cmd/auth_m365_link.gointernal/cmd/m365_login_link_test.go
2 participants
Summary
Wires the M365 one-click broker into Workit's auth-server so Felipe can deploy it as a tenant-owned pod inside Hapvida/OCI HV.
What changed
wk auth m365 login-linknow uses dynamic callback-server defaults when URLs are omitted:WK_CALLBACK_SERVER/ configured callback server becomes broker base URL<base>/m365/callback--base-url/--callback-urlstill overrideauth-server:POST /m365/sessionsGET /m365/start/:stateGET /m365/callbackWK_PUBLIC_BASE_URL/--public-base-urlWK_M365_CLIENT_ID/--m365-client-idWK_M365_TENANT_ID/--m365-tenant-idWK_CALLBACK_SERVERremains backward-compatible public base fallbackauth-server/Dockerfilefor OCI/HV pod deployment.make build-auth-servermake docker-auth-serverSafety
Mail.Send,Calendars.ReadWriteabsent)./meemail/UPN against expected email before storing token./token/:staterelay path after successful callback.Verification
Results:
Dogfood local auth-server:
./bin/workit-auth-serverwith:WK_PUBLIC_BASE_URL=https://auth.hv.exampleWK_M365_CLIENT_IDWK_M365_TENANT_ID=hapvida-tenantPOST /m365/sessionsreturned:login_url=https://auth.hv.example/m365/start/<state>expected_email=bernardo@hapvida.com.brcode_verifierGET /m365/start/<state>returned 302 tologin.microsoftonline.comredirect_uri=https://auth.hv.example/m365/callbackcode_challenge_method=S256Docker note: Dockerfile is committed and covered by a config test, but this runner cannot connect to
/var/run/docker.sock(permission denied), so image build must be run by CI or an environment with Docker daemon access.Refs #45.
Summary by CodeRabbit
New Features
/m365/sessions,/m365/start/{state}, and/m365/callback.Documentation
Chores