fix: resolve GitHub OAuth integration bug chain#3355
Open
marcusgrando wants to merge 6 commits intodevfrom
Open
fix: resolve GitHub OAuth integration bug chain#3355marcusgrando wants to merge 6 commits intodevfrom
marcusgrando wants to merge 6 commits intodevfrom
Conversation
a845220 to
e0dc476
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes a chain of interconnected bugs in the GitHub OAuth integration flow that affected template deployment, account settings, and the import GitHub page. The bugs caused silent failures, missing error feedback, and a broken popup lifecycle.
Bug Analysis & Fixes
Bug 1 (ROOT CAUSE): GitHub integrations not loaded on
engine-azionmountProblem: When a template page with a VCS integration field (
platform_feature__vcs_integration__uuid) was loaded for the first time, the GitHub integrations dropdown was empty. The user had to navigate away and come back (triggering thewatchonprops.schema) for integrations to appear.Root cause: The
onMountedhook inengine-azion.vueonly calledinitializeForm()but never checked whether the schema contained a VCS field that required loading integrations. The integration loading logic (loadIntegrationOnShowButton) only existed inside thewatchhandler — meaning it only fired on schema changes, not on the initial mount.Fix: After
initializeForm()completes inonMounted, we now extract field names frominputSchema.value.groupsand check if the VCS integration field is present. If so,loadIntegrationOnShowButton()is called immediately, loading the integrations list and registering the message event listener.Why this approach: We reuse the existing
extractFieldNames+loadIntegrationOnShowButtonfunctions — no new abstractions. The same pattern already worked correctly in thewatchhandler and inengine-jsonform.vue'sonMounted, so this alignsengine-azionwith the established convention.Files:
src/templates/template-engine-block/engine-azion.vueBug 2: Popup always sent
integration-dataregardless of callback typeProblem: The
GitHubConnectionPopupcomponent always posted a{ event: 'integration-data', data: route.query }message to the opener window, regardless of whether the OAuth flow returned acode(authorization), anerror(user denied access), or aninstallation_id(GitHub App installation without OAuth code).This meant:
code, justinstallation_id+setup_action) were incorrectly treated as OAuth authorization data, causing the parent to attemptpostCallbackUrlwith wrong parameters → HTTP 400.?error=access_denied&error_description=...) were also sent asintegration-data, causing the parent to silently fail with no user feedback.Fix: The popup now inspects
route.queryand dispatches one of three explicit events:route.query.codeexistsintegration-datapostCallbackUrlroute.query.errorexistsintegration-errorintegration-connectedWhy this approach: The three GitHub OAuth/App callback shapes are well-documented and mutually exclusive. Using explicit event names makes the contract between popup and parent unambiguous and easy to debug. No new dependencies — just branching on existing query parameters.
Files:
src/views/GitHubConnectionPopup/index.vueBug 3: Toast error used
error.detailinstead oferror.messageProblem: In 4 files, the
showWithOptionscallback accessederror.detailto build the toast summary:summary: `GitHub integration failed: ${error.detail}`However, the error objects from the API service layer expose the error text on
error.message, noterror.detail. This resulted in toast messages showingGitHub integration failed: undefined— providing zero useful information to the user.Fix: Replace
error.detailwitherror.messagein all 4 occurrences.Why this approach: The error objects are standard JavaScript
Errorinstances (or extend them). The.messageproperty is the canonical way to access error text per the ECMAScript specification. The.detailproperty does not exist on these objects.Files:
src/templates/template-engine-block/engine-azion.vuesrc/templates/template-engine-block/engine-jsonform.vuesrc/views/ImportGitHub/FormFields/FormFieldsImportGithub.vuesrc/views/AccountSettings/FormFields/FormFieldsAccountSettings.vueBug 4: Popup used
window.parent.close()instead ofwindow.close()Problem: The popup called
window.parent.close()after posting the message. Since the popup is opened viawindow.open()(not an iframe),window.parentrefers to the popup's own window — but semantically this is incorrect and can behave unexpectedly across browsers. In some contexts,window.parent.close()may be blocked by the browser's popup policy.Fix: Changed to
window.close(), which is the correct API for a popup window to close itself.Files:
src/views/GitHubConnectionPopup/index.vueHardening: Origin validation on message listeners
Problem: All 4 message listener handlers accepted
postMessageevents from any origin. This is a security vulnerability — a malicious page could send crafted messages to the console-kit window and trigger integration saves or list reloads.Fix: Every
messageevent handler now starts with an origin guard:This ensures only messages from the same origin (the platform itself, including the popup) are processed.
Why
window.location.origin: The popup is always on the same domain as the parent. The popup already useswindow.location.originas thetargetOriginparameter inpostMessage, so this is a symmetric check.Files:
src/templates/template-engine-block/engine-azion.vuesrc/templates/template-engine-block/engine-jsonform.vuesrc/views/ImportGitHub/FormFields/FormFieldsImportGithub.vuesrc/views/AccountSettings/FormFields/FormFieldsAccountSettings.vueHardening: Listeners now handle all 3 popup event types
Problem: The
messageevent handlers only checked forintegration-data. After the popup fix (Bug 2), the popup can now sendintegration-connectedandintegration-errorevents, but without updating the listeners, these events would be silently ignored.Fix: All 4 listener handlers now respond to all 3 events:
integration-data→ callsaveIntegration(event.data)(existing behavior)integration-connected→ reload the integrations list (fixes "Add GitHub Account" when already installed)integration-error→ show error toast to the userFiles: Same 4 listener files as above.
Test Plan
Automated tests added
src/tests/templates/template-engine-block/engine-azion-mount-integrations.test.jslistIntegrationsis called on mount when VCS field exists in schema groupslistIntegrationsis NOT called when VCS field is absentsrc/tests/views/GitHubConnectionPopup/index.test.jsintegration-dataevent sent whencodequery param existsintegration-connectedevent sent when nocodeand noerrorintegration-errorevent sent whenerrorquery param existswindow.close()is called after posting messageManual verification checklist
codepersists integration and refreshes listundefined)