[WEB-4240,WEB-4241] Add Oura authentication#1854
[WEB-4240,WEB-4241] Add Oura authentication#1854clintonium-119 wants to merge 29 commits intodevelopfrom
Conversation
- Add Oura authentication - Add provider flag to require the patient must be logged in to connect - Add provider flag to require an existing data source to connect - Filter active providers for only allowed connections based on above - Add new accept status to the OAuth workflow for UI before authorization - Add Oura logo
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR adds OAuth authentication support for the Oura health device provider and introduces new provider configuration flags to control connection requirements. The implementation includes a new pre-authorization "accept" step in the OAuth flow that allows for consent collection before redirecting users to the provider's authorization page.
Changes:
- Added Oura as a new OAuth provider with logo and configuration
- Introduced provider flags (
requiresLoggedInUser,requiresExistingDataSource) to restrict connection eligibility - Added new "accept" status to OAuth workflow for pre-authorization consent step
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| app/pages/oauth/OAuthConnection.js | Added "accept" status with UI and redirect handler for pre-authorization step |
| app/core/icons/oura_logo.svg | Added Oura logo SVG file |
| app/components/datasources/useProviderConnectionPopup.js | Added early return for "accept" status to prevent premature popup closure |
| app/components/datasources/DataConnections.js | Added Oura provider configuration with new restriction flags and updated filtering logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…vey the state to the user
Restructured the consent dialog layout for full-width dividers and proper content alignment per design specifications. - Moved padding from wrapper to individual sections (header/content/footer) - Image now sits inline with subheading only, body text below full-width - Right-justified the "I understand" button in footer
Wrapped first consent paragraph with Trans component to make "Tidepool's Privacy Policy" a clickable link. - Added URL_PRIVACY_POLICY import from constants - Used html.oauth-accept-privacy-message i18n key for Trans - Updated message rendering to handle both string and JSX
Added check to ensure return_url starts with '/' before redirecting, protecting against potential open redirect attacks via crafted URLs.
|
|
||
| // Validate return_url is a relative path to prevent open redirect attacks | ||
| if (returnUrl?.startsWith('/')) { | ||
| window.location.href = returnUrl; |
Check warning
Code scanning / CodeQL
Client-side URL redirect Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
In general, this problem is fixed by not redirecting to arbitrary URLs derived from user input. Instead, accept only safe intra-site paths or use a strict allowlist of known redirect targets. For client-side enforcement, you should normalize the candidate URL using the current origin, reject anything that changes origin or protocol, and ideally only allow relative paths (e.g., starting with /) that stay within your application.
For this specific file (app/pages/oauth/OAuthConnection.js), the single best fix is to tighten and simplify isSafeReturnUrl so it only allows same-origin, same-protocol, path-only URLs that begin with /. We can do this by:
- Treating URLs that begin with
/as path-only and resolving them againstwindow.location.origin. - Rejecting URLs that contain a protocol, host, or port different from the current page.
- Ensuring we only accept
http:orhttps:and that the normalized URL’s origin exactly matcheswindow.location.origin. - Returning
falseon any parsing error.
Then, we keep handleAccept logic the same, but ensure it only redirects if the strengthened validator approves the URL. No changes to other behavior are needed. All changes stay within this file, only in the isSafeReturnUrl helper. No new imports are required.
Concretely:
- Update the implementation of
isSafeReturnUrl(lines 96–111) to:- Immediately reject empty values.
- Normalize the candidate URL with
new URL(url, window.location.origin). - Require
returnUrl.origin === window.location.origin. - Require protocol to be
http:orhttps:. - Require that the URL was effectively a path within this origin (we can enforce that the original value starts with
/and does not contain//before the path, or simply rely on origin equality plus a leading/constraint).
- Keep
handleAcceptas-is, so thatwindow.location.hrefis only ever assigned a safe, same-origin path.
| @@ -97,9 +97,14 @@ | ||
| if (!url) return false; | ||
|
|
||
| try { | ||
| // Only allow absolute paths within the current origin, e.g. "/path?query" | ||
| // Reject values that do not start with "/" to avoid protocol-relative or malformed URLs. | ||
| if (!url.startsWith('/')) { | ||
| return false; | ||
| } | ||
|
|
||
| const returnUrl = new URL(url, window.location.origin); | ||
|
|
||
| // Enforce same-origin and restrict to http/https protocols and absolute paths | ||
| const isSameOrigin = returnUrl.origin === window.location.origin; | ||
| const isHttpProtocol = returnUrl.protocol === 'http:' || returnUrl.protocol === 'https:'; | ||
| const isAbsolutePath = returnUrl.pathname.startsWith('/'); |
Improved return_url validation with isSafeReturnUrl helper to prevent open redirect attacks. Added test coverage for new provider features. - Added isSafeReturnUrl with checks for protocol-relative URLs - Added oura provider to test expectations - Added tests for requiresLoggedInUser filtering - Added tests for requiresExistingDataSource filtering - Added test for accept status early return in popup - Updated AGENTS.md with targeted test running guidance
|
|
||
| // Validate return_url is a safe relative path to prevent open redirect attacks | ||
| if (isSafeReturnUrl(returnUrl)) { | ||
| window.location.href = returnUrl; |
Check failure
Code scanning / CodeQL
Client-side cross-site scripting High
This autofix suggestion was applied.
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 months ago
In general, the safest pattern is to treat return_url as an application-internal relative path, not an arbitrary URL, and to enforce strict validation before using it for navigation. That means: (1) reject missing values; (2) parse with a known base (window.location.origin); (3) ensure the resulting URL is same-origin; (4) ensure it uses http or https and not any custom or JavaScript schemes; and (5) ideally, ensure it is a path you expect (e.g., starts with /).
The best fix here, without changing external behavior beyond tighter security, is to harden isSafeReturnUrl so that it explicitly rejects any value that does not resolve to an http or https URL on the current origin, with a path that starts with /. This keeps existing functionality (allowing same-origin relative URLs) but blocks unsafe schemes or external redirects. The only code to change is inside app/pages/oauth/OAuthConnection.js, specifically the isSafeReturnUrl helper. We do not need new imports or new functions beyond updating this existing one.
Concretely:
- Modify
isSafeReturnUrlto:- Immediately return
falseifurlis falsy (null/undefined/empty). - Construct
const returnUrl = new URL(url, window.location.origin); - Check that
returnUrl.origin === window.location.origin. - Check that
returnUrl.protocolishttp:orhttps:. - Check that
returnUrl.pathnamestarts with/.
- Immediately return
- Leave
handleAcceptunchanged; it will now rely on the stricter validator.
| @@ -92,10 +92,17 @@ | ||
| }, []); | ||
|
|
||
| const isSafeReturnUrl = (url) => { | ||
| if (!url) return false; | ||
|
|
||
| try { | ||
| // Must be on the same origin | ||
| const returnUrl = new URL(url, window.location.origin); | ||
| return returnUrl.origin === window.location.origin; | ||
|
|
||
| // Enforce same-origin and restrict to http/https protocols and absolute paths | ||
| const isSameOrigin = returnUrl.origin === window.location.origin; | ||
| const isHttpProtocol = returnUrl.protocol === 'http:' || returnUrl.protocol === 'https:'; | ||
| const isAbsolutePath = returnUrl.pathname.startsWith('/'); | ||
|
|
||
| return isSameOrigin && isHttpProtocol && isAbsolutePath; | ||
| } catch (e) { | ||
| return false; | ||
| } |
clintonium-119
committed this autofix suggestion 2 months ago.
Fixed tests to account for oura's requiresLoggedInUser and requiresExistingDataSource restrictions. Updated AGENTS.md to use .only for isolating Karma tests instead of --grep.
|
/deploy qa4 |
|
clintonium-119 updated values.yaml file in qa4 |
|
clintonium-119 updated flux policies file in qa4 |
|
clintonium-119 deployed blip WEB-4240-oura-connections branch to qa4 namespace |
clintonium-119
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 13 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
app/pages/oauth/OAuthConnection.js:79
- The statusContent object is defined inside the component body and is recreated on every render. This could cause unnecessary re-renders and memory allocation. Since it depends on the
tanddisplayNamevalues, consider usinguseMemoto memoize it and prevent recreation unless these dependencies change.
const statusContent = {
accept: {
status: 'accept',
title: t('Your Body, Your Data'),
subheading: t('Your {{displayName}} data may contain information about your reproductive health', { displayName }),
message: [
<Trans i18nKey="html.oauth-accept-privacy-message">
<Body1 mb={3}>
As part of using Tidepool Apps and Services or as part of certain initiatives, we may collect reproductive health data that you provide to us or that you authorize a 3rd Party to provide to us on your behalf. This data may be used and disclosed in accordance with <Link href={URL_PRIVACY_POLICY} target="_blank" rel="noreferrer noopener">Tidepool's Privacy Policy</Link> and applicable law. You can stop sharing reproductive health data at any point by accessing your Devices page and disconnecting 3rd Party devices, which share these data to Tidepool. You may also request your data be deleted at any time. Please see 1.2.4 Export, Delete, or Change Your Information and 1.2.5 Cancel Your Account in our Privacy Policy.
</Body1>
</Trans>,
t('By linking your {{ displayName }} account, you acknowledge that Tidepool may collect, use, and disclose data derived from your device, including reproductive health data. Please consider the laws governing reproductive health in your jurisdiction before providing Tidepool with such data.', { displayName }),
],
image: consentDataImage,
},
authorized: {
status: 'authorized',
title: t('Connection Authorized'),
subheading: t('Thank you for connecting with Tidepool!'),
message: [t('We hope you enjoy your Tidepool experience.')],
banner: {
message: t('You have successfully connected your {{displayName}} data to Tidepool.', { displayName }),
variant: 'success',
},
},
declined: {
status: 'declined',
title: t('Connection Declined'),
subheading: t('You can always decide to connect at a later time.'),
message: [t('We hope you enjoy your Tidepool experience.')],
banner: {
message: t('You have declined connecting your {{displayName}} data to Tidepool.', { displayName }),
variant: 'info',
},
},
error: {
status: 'error',
title: t('Connection Error'),
subheading: t('Hmm... That didn\'t work. Please try again.'),
banner: {
message: t('We were unable to determine your {{displayName}} connection status.', { displayName }),
variant: 'danger',
},
},
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const [activeHandler, setActiveHandler] = useState(null); | ||
| const dataConnectionProps = getDataConnectionProps(patient, isLoggedInUser, selectedClinicId, setActiveHandler); | ||
| const activeProviders = getActiveProviders(); | ||
| const activeProviders = filter(getActiveProviders(), providerName => !!dataConnectionProps[providerName]); // Only those with connection props |
There was a problem hiding this comment.
The double negation !!dataConnectionProps[providerName] is unnecessary here. Since this is being used in a boolean context with the filter function, you can simplify it to just dataConnectionProps[providerName] for better readability.
| const activeProviders = filter(getActiveProviders(), providerName => !!dataConnectionProps[providerName]); // Only those with connection props | |
| const activeProviders = filter(getActiveProviders(), providerName => dataConnectionProps[providerName]); // Only those with connection props |
| As part of using Tidepool Apps and Services or as part of certain initiatives, we may collect reproductive health data that you provide to us or that you authorize a 3rd Party to provide to us on your behalf. This data may be used and disclosed in accordance with <Link href={URL_PRIVACY_POLICY} target="_blank" rel="noreferrer noopener">Tidepool's Privacy Policy</Link> and applicable law. You can stop sharing reproductive health data at any point by accessing your Devices page and disconnecting 3rd Party devices, which share these data to Tidepool. You may also request your data be deleted at any time. Please see 1.2.4 Export, Delete, or Change Your Information and 1.2.5 Cancel Your Account in our Privacy Policy. | ||
| </Body1> | ||
| </Trans>, | ||
| t('By linking your {{ displayName }} account, you acknowledge that Tidepool may collect, use, and disclose data derived from your device, including reproductive health data. Please consider the laws governing reproductive health in your jurisdiction before providing Tidepool with such data.', { displayName }), |
There was a problem hiding this comment.
Inconsistent spacing in the translation template string interpolation. On line 39, the pattern is {{displayName}} (no spaces inside braces), but on line 46, the pattern is {{ displayName }} (spaces inside braces). For consistency, use the same spacing pattern throughout the file.
| t('By linking your {{ displayName }} account, you acknowledge that Tidepool may collect, use, and disclose data derived from your device, including reproductive health data. Please consider the laws governing reproductive health in your jurisdiction before providing Tidepool with such data.', { displayName }), | |
| t('By linking your {{displayName}} account, you acknowledge that Tidepool may collect, use, and disclose data derived from your device, including reproductive health data. Please consider the laws governing reproductive health in your jurisdiction before providing Tidepool with such data.', { displayName }), |
| import { push } from 'connected-react-router'; | ||
| import { withTranslation, Trans } from 'react-i18next'; | ||
| import { useParams, useLocation } from 'react-router-dom'; | ||
| import capitalize from 'lodash/capitalize'; | ||
| import includes from 'lodash/includes'; | ||
| import { Box, Flex } from 'theme-ui'; | ||
| import map from 'lodash/map'; | ||
| import { Box, Flex, Divider, Image, Link } from 'theme-ui'; | ||
| import { components as vizComponents } from '@tidepool/viz'; | ||
| import utils from '../../core/utils'; |
There was a problem hiding this comment.
Import ordering does not follow the project conventions specified in AGENTS.md. According to the style guide, imports should be grouped with blank lines between: (1) React, (2) PropTypes, (3) Redux, (4) Third-party libraries, (5) Lodash, (6) theme-ui, (7) Local imports. Currently, the Redux import (push from connected-react-router) is mixed with react-i18next and react-router-dom. Move the push import after the useDispatch import from react-redux and group them together. Also, add blank lines between the import groups.
| } else { | ||
| // eslint-disable-next-line no-console | ||
| console.error('OAuthConnection: invalid or missing return_url query parameter on accept.'); |
There was a problem hiding this comment.
When the return_url validation fails, the function logs an error but does not provide user feedback or alternative behavior. The user will see a loading state indefinitely because acceptProcessing is set to true. Consider either resetting the processing state, showing an error toast, or redirecting to a fallback path when validation fails.
| } else { | |
| // eslint-disable-next-line no-console | |
| console.error('OAuthConnection: invalid or missing return_url query parameter on accept.'); | |
| } else { | |
| // If the return_url is invalid, stop the processing state and redirect to a safe fallback. | |
| setAcceptProcessing(false); | |
| // eslint-disable-next-line no-console | |
| console.error('OAuthConnection: invalid or missing return_url query parameter on accept.'); | |
| dispatch(push('/patients')); |
| if (!!provider.requiresLoggedInUser && !isLoggedInUser) { | ||
| return result; | ||
| } | ||
|
|
||
| // If the provider requires an existing data source to create the connection, then ensure that is the case. | ||
| // This mechanism can be used to limit access to certain providers to only users who have previously connected | ||
| // or where Tidepool has created a data source on their behalf. | ||
| if (!!provider.requiresExistingDataSource && !dataSource) { |
There was a problem hiding this comment.
The double negation !!provider.requiresLoggedInUser is unnecessary here. Since this is being used in a boolean context, you can simplify it to just provider.requiresLoggedInUser for better readability.
| if (!!provider.requiresLoggedInUser && !isLoggedInUser) { | |
| return result; | |
| } | |
| // If the provider requires an existing data source to create the connection, then ensure that is the case. | |
| // This mechanism can be used to limit access to certain providers to only users who have previously connected | |
| // or where Tidepool has created a data source on their behalf. | |
| if (!!provider.requiresExistingDataSource && !dataSource) { | |
| if (provider.requiresLoggedInUser && !isLoggedInUser) { | |
| return result; | |
| } | |
| // If the provider requires an existing data source to create the connection, then ensure that is the case. | |
| // This mechanism can be used to limit access to certain providers to only users who have previously connected | |
| // or where Tidepool has created a data source on their behalf. | |
| if (provider.requiresExistingDataSource && !dataSource) { |
There was a problem hiding this comment.
I don't actually mind the boolean coerce with the !!. If we had good type enforcement/inference, I might feel differently, but I generally lean into the defensive stance on these things.
|
|
||
| it('should not close popup or show a toast message when the authorization status is `accept`', (done) => { | ||
| // Simulate pre-authorization accept path - popup should remain open | ||
| const authorizedDataSource = { id: 'oauth/testProvider', url: `${window.location.origin}/oauth/testProvider/accept`}; |
There was a problem hiding this comment.
Missing space after colon in the object literal. Should be url: with a space after the colon to follow consistent code style conventions.
|
|
||
| // Validate return_url is a safe relative path to prevent open redirect attacks | ||
| if (isSafeReturnUrl(returnUrl)) { | ||
| window.location.href = returnUrl; |
There was a problem hiding this comment.
@clintonium-119 since the automated scans seem really upset by this, could something that uses the URL primitive to make the checks more complete/robust like:
try {
// Must be on the same origin
const returnUrl = new URL(url, window.location.origin);
return returnUrl.origin === window.location.origin;
} catch (e) {
return false;
}
…scripting Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 13 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| window.location.href = returnUrl; | ||
| } else { | ||
| // eslint-disable-next-line no-console | ||
| console.error('OAuthConnection: invalid or missing return_url query parameter on accept.'); |
There was a problem hiding this comment.
handleAccept sets acceptProcessing to true before validating return_url, but in the invalid/missing case it only logs an error and never resets acceptProcessing or provides a user-visible fallback. This can leave the UI stuck in a loading state with no way forward if the query param is absent/malformed. Consider resetting acceptProcessing and redirecting to a safe default (e.g., the in-app route used by handleRedirectToTidepool) or showing an error state/toast so the user can recover.
| console.error('OAuthConnection: invalid or missing return_url query parameter on accept.'); | |
| console.error('OAuthConnection: invalid or missing return_url query parameter on accept.'); | |
| // Reset processing state and redirect to a safe in-app route so the user can recover. | |
| setAcceptProcessing(false); | |
| let path = '/patients?justLoggedIn=true' | |
| + `&dataConnectionStatus=${status}` | |
| + `&dataConnectionProviderName=${providerName}`; | |
| dispatch(push(path)); |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Hey @krystophv Just pinging for final review. I've implemented the URL generation approach you'd recommended with a couple of extra safeguards. |
|
/deploy qa5 |
|
clintonium-119 updated values.yaml file in qa5 |
|
clintonium-119 updated flux policies file in qa5 |
|
clintonium-119 deployed blip WEB-4240-oura-connections branch to qa5 namespace |
The scroll handler only sets the ConsentRead flag when the user scrolls to the bottom of the consent document. If the content is short enough that the container isn't scrollable, the scroll event never fires and the user has no way to enable the consent checkbox. Attach a ref to the scroll container and run an effect that compares scrollHeight to clientHeight; if the content fits without scrolling, mark the current step's ConsentRead field as true. Guarded against re-setting an already-true value to avoid a Formik update loop.
…nsent - When the consent document is short enough that the container isn't scrollable, the scroll handler never fires and the consent checkbox stays disabled. Capture the scroll container via a useState setter ref so MUI Portal's deferred child mount triggers a re-render, then in a useEffect mark the current step's ConsentRead as true if scrollHeight fits within clientHeight. Guarded against already-true values (prevents Formik update loops) and zero-height layout (jsdom). - Include the name field in the primary schema for child patients on personal accounts (previously only enforced for youth).
|
/deploy qa5 |
|
clintonium-119 updated values.yaml file in qa5 |
|
clintonium-119 updated flux policies file in qa5 |
|
clintonium-119 deployed blip WEB-4240-oura-connections branch to qa5 namespace |
WEB-4240 WEB-4241
Also, I've added an AGENTS.md similar to the one added to viz to provide some basic guidelines for opencode to read for initial agent context.