mcgilman commented on code in PR #11254: URL: https://github.com/apache/nifi/pull/11254#discussion_r3262904402
########## nifi-frontend/src/main/frontend/apps/nifi/src/app/pages/connectors/ui/connector-detail/connector-detail.component.ts: ########## @@ -0,0 +1,133 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { Component, DestroyRef, ElementRef, OnInit, SecurityContext, viewChild, inject } from '@angular/core'; +import { HttpErrorResponse } from '@angular/common/http'; +import { Router } from '@angular/router'; +import { NiFiState } from '../../../../state'; +import { Store } from '@ngrx/store'; +import { DomSanitizer, SafeResourceUrl } from '@angular/platform-browser'; +import { takeUntilDestroyed } from '@angular/core/rxjs-interop'; +import { Navigation } from '../../../../ui/common/navigation/navigation.component'; +import { + SystemTokensService, + ConnectorEntity, + ConnectorConfigurationService, + NifiSpinnerDirective +} from '@nifi/shared'; +import { ConnectorDetailsContent } from '../connector-details-content/connector-details-content.component'; +import { catchError, filter, switchMap } from 'rxjs/operators'; +import { selectConnectorIdFromRoute } from '../../state/connectors-listing/connectors-listing.selectors'; +import { MatButton } from '@angular/material/button'; +import { ErrorHelper } from '../../../../service/error-helper.service'; +import { ContextErrorBanner } from '../../../../ui/common/context-error-banner/context-error-banner.component'; +import { ErrorContextKey } from '../../../../state/error'; +import { selectBackNavigation } from '../../../../state/navigation/navigation.selectors'; +import { of } from 'rxjs'; +import { ConnectorMessageHost } from '../../service/connector-message-host.service'; + +@Component({ + selector: 'connector-detail', + imports: [Navigation, ConnectorDetailsContent, MatButton, NifiSpinnerDirective, ContextErrorBanner], + templateUrl: './connector-detail.component.html', + styleUrls: ['./connector-detail.component.scss'] +}) +export class ConnectorDetail implements OnInit { + private store = inject<Store<NiFiState>>(Store); + private router = inject(Router); + private domSanitizer = inject(DomSanitizer); + private connectorConfigurationService = inject(ConnectorConfigurationService); + private destroyRef = inject(DestroyRef); + private errorHelper = inject(ErrorHelper); + protected systemTokensService = inject(SystemTokensService); + private connectorMessageHost = inject(ConnectorMessageHost); + + readonly iframeRef = viewChild<ElementRef<HTMLIFrameElement>>('iframeRef'); + + backNavigation = this.store.selectSignal(selectBackNavigation); + connectorIdFromRoute = this.store.selectSignal(selectConnectorIdFromRoute); + frameSource: SafeResourceUrl | null = null; + connector: ConnectorEntity | null = null; + loading = true; + errorMessage: string | null = null; + + protected readonly ErrorContextKey = ErrorContextKey; + + ngOnInit(): void { + this.store + .select(selectConnectorIdFromRoute) + .pipe( + filter((connectorId) => connectorId != null), + switchMap((connectorId) => { + this.loading = true; + this.frameSource = null; + this.connector = null; + this.errorMessage = null; + + return this.connectorConfigurationService.getConnector(connectorId!).pipe( + catchError((errorResponse: HttpErrorResponse) => { + this.loading = false; + this.errorMessage = this.errorHelper.getErrorString(errorResponse); + return of(null); + }) + ); + }), + takeUntilDestroyed(this.destroyRef) + ) + .subscribe((connector: ConnectorEntity | null) => { + if (!connector) { + return; + } + + this.connector = connector; + this.loading = false; + this.errorMessage = null; + + if (!connector.permissions.canRead) { + this.errorMessage = 'Insufficient permissions to view this connector.'; + return; + } + + if (connector.component?.detailsUrl) { + this.frameSource = this.getFrameSource(connector.component.detailsUrl); + + this.connectorMessageHost.startListening({ + destroyRef: this.destroyRef, + expectedOrigin: ConnectorMessageHost.extractOrigin(connector.component.detailsUrl), + iframeElement: () => this.iframeRef()?.nativeElement + }); + } + }); + } + + private getFrameSource(detailsUrl: string): SafeResourceUrl | null { + const connectorId = this.connector?.id; + const urlWithParams = connectorId ? `${detailsUrl}?connectorId=${connectorId}` : detailsUrl; + + const sanitizedUrl = this.domSanitizer.sanitize(SecurityContext.URL, urlWithParams); + + if (sanitizedUrl) { Review Comment: `DomSanitizer.sanitize(SecurityContext.URL, …)` doesn't return `null` for unsafe schemes — it returns the original string with an `'unsafe:'` prefix. So for `'javascript:alert(1)'` it returns the truthy string `'unsafe:javascript:alert(1)'`, `if (sanitizedUrl)` passes, and we hand a `SafeResourceUrl` containing that string to `[src]`. The page is safe today only because no browser recognizes `unsafe:` as a scheme — not because this method rejected anything. One-liner fix — either prefix-check: ```ts if (!sanitizedUrl || sanitizedUrl.startsWith('unsafe:')) { return null; } return this.domSanitizer.bypassSecurityTrustResourceUrl(sanitizedUrl); ``` …or scheme-validate via `new URL(...).protocol` against `http:`/`https:`, which is more robust against future Angular sanitizer changes. Same drift exists in `ConnectorConfigure.getFrameSource` — worth fixing in both files so we don't keep duplicating a misleading helper. If you take this change, the `'should mark invalid URLs as unsafe'` spec becomes `expect(result).toBeNull()`. ########## nifi-frontend/src/main/frontend/apps/nifi/src/app/pages/connectors/ui/connector-detail/connector-detail.component.html: ########## @@ -0,0 +1,66 @@ +<!-- + ~ Licensed to the Apache Software Foundation (ASF) under one or more + ~ contributor license agreements. See the NOTICE file distributed with + ~ this work for additional information regarding copyright ownership. + ~ The ASF licenses this file to You under the Apache License, Version 2.0 + ~ (the "License"); you may not use this file except in compliance with + ~ the License. You may obtain a copy of the License at + ~ + ~ http://www.apache.org/licenses/LICENSE-2.0 + ~ + ~ Unless required by applicable law or agreed to in writing, software + ~ distributed under the License is distributed on an "AS IS" BASIS, + ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + ~ See the License for the specific language governing permissions and + ~ limitations under the License. + --> + +<div class="flex flex-col h-full"> + <header class="nifi-header"> + <navigation></navigation> + </header> + @if (!backNavigation() && !loading && !errorMessage) { + <div class="pl-5 pt-2"> + <a (click)="returnToConnectorListing()" data-qa="fallback-back-link"> + <i class="fa fa-arrow-left mr-2" aria-hidden="true"></i>Installed connectors + </a> + </div> + } + <context-error-banner [context]="ErrorContextKey.CONNECTORS"></context-error-banner> + <div class="flex flex-1 min-h-0 overflow-y-auto"> + @if (loading) { + <div class="flex-1 flex items-center justify-center"> + <span *nifiSpinner="loading" data-qa="loading-spinner"></span> + </div> + } @else if (errorMessage) { + <div class="flex-1 flex items-center justify-center" data-qa="error-container"> + <div class="flex flex-col items-center gap-y-4"> + <i class="fa fa-warning error-color text-4xl" data-qa="error-icon" aria-hidden="true"></i> + <div data-qa="error-message">{{ errorMessage }}</div> Review Comment: Minor consistency: `ConnectorConfigure` uses `<p class="text-base" data-qa="error-message">{{ errorMessage }}</p>` for the same role. Without the `<p>` + `text-base`, the error text picks up the surrounding default typography instead of Material body sizing. Suggest: ```html <p class="text-base" data-qa="error-message">{{ errorMessage }}</p> ``` ########## nifi-frontend/src/main/frontend/apps/nifi/src/app/pages/connectors/ui/connector-detail/connector-detail.component.ts: ########## @@ -0,0 +1,133 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { Component, DestroyRef, ElementRef, OnInit, SecurityContext, viewChild, inject } from '@angular/core'; +import { HttpErrorResponse } from '@angular/common/http'; +import { Router } from '@angular/router'; +import { NiFiState } from '../../../../state'; +import { Store } from '@ngrx/store'; +import { DomSanitizer, SafeResourceUrl } from '@angular/platform-browser'; +import { takeUntilDestroyed } from '@angular/core/rxjs-interop'; +import { Navigation } from '../../../../ui/common/navigation/navigation.component'; +import { + SystemTokensService, + ConnectorEntity, + ConnectorConfigurationService, + NifiSpinnerDirective +} from '@nifi/shared'; +import { ConnectorDetailsContent } from '../connector-details-content/connector-details-content.component'; +import { catchError, filter, switchMap } from 'rxjs/operators'; +import { selectConnectorIdFromRoute } from '../../state/connectors-listing/connectors-listing.selectors'; +import { MatButton } from '@angular/material/button'; +import { ErrorHelper } from '../../../../service/error-helper.service'; +import { ContextErrorBanner } from '../../../../ui/common/context-error-banner/context-error-banner.component'; +import { ErrorContextKey } from '../../../../state/error'; +import { selectBackNavigation } from '../../../../state/navigation/navigation.selectors'; +import { of } from 'rxjs'; +import { ConnectorMessageHost } from '../../service/connector-message-host.service'; + +@Component({ + selector: 'connector-detail', + imports: [Navigation, ConnectorDetailsContent, MatButton, NifiSpinnerDirective, ContextErrorBanner], + templateUrl: './connector-detail.component.html', + styleUrls: ['./connector-detail.component.scss'] +}) +export class ConnectorDetail implements OnInit { + private store = inject<Store<NiFiState>>(Store); + private router = inject(Router); + private domSanitizer = inject(DomSanitizer); + private connectorConfigurationService = inject(ConnectorConfigurationService); + private destroyRef = inject(DestroyRef); + private errorHelper = inject(ErrorHelper); + protected systemTokensService = inject(SystemTokensService); + private connectorMessageHost = inject(ConnectorMessageHost); + + readonly iframeRef = viewChild<ElementRef<HTMLIFrameElement>>('iframeRef'); + + backNavigation = this.store.selectSignal(selectBackNavigation); + connectorIdFromRoute = this.store.selectSignal(selectConnectorIdFromRoute); + frameSource: SafeResourceUrl | null = null; + connector: ConnectorEntity | null = null; + loading = true; + errorMessage: string | null = null; + + protected readonly ErrorContextKey = ErrorContextKey; + + ngOnInit(): void { + this.store + .select(selectConnectorIdFromRoute) + .pipe( + filter((connectorId) => connectorId != null), + switchMap((connectorId) => { + this.loading = true; + this.frameSource = null; Review Comment: Nit, not a bug: this is safe today because `startListening()` internally calls `stopListening()` and everything is anchored to `DestroyRef`. The one edge case is navigating from an iframe connector to a non-iframe connector — the previous subscription stays bound until the component is destroyed (harmless, since origin/source checks fail against a detached `contentWindow`). A single `this.connectorMessageHost.stopListening()` here would make intent obvious. Optional. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
