From 3c68ac285084bb822a26ec7fbefdd5b2b735fcc3 Mon Sep 17 00:00:00 2001 From: cproudlock Date: Wed, 26 Aug 2026 15:05:30 -0400 Subject: [PATCH] ADR-015: the site-literal check was matching nothing, and had been all along The patterns are assembled from fragments so the published script does not contain the strings it greps for. They used the apostrophe-escape form ('a'\''b'), which EMBEDS a literal apostrophe, instead of the concatenation form ('a''b'), which joins them. Every pattern therefore carried a stray quote in the middle and could never match a line of real code, so the check passed everything from the day it was written. Verified the only way that means anything: plant a violation, watch it fail, revert. Fixed, it immediately found 25 real hits. Among them, in seven places, the default ServiceNow URLs - one organisation's QA tenant, shipped with the integration ENABLED. Any site that had not configured ServiceNow still rendered ticket links, pointing its technicians at a tenant they cannot reach; an adopting site would have been pointed into somebody else's instance entirely. There is no sensible cross-site default for "which ServiceNow is yours", so the URLs ship blank and the integration ships off, matching the Zabbix block that got this right. A named SaaS tenant is as site-specific as a hostname, so it joins the patterns. Colocated .spec.js files are excluded the same way tests/ already is: sample data naming a facility is not a shipped literal. --- frontend/src/composables/pageTitle.js | 4 +-- frontend/src/utils/siteSettings.js | 26 ++++++++-------- .../src/views/settings/ServiceNowSettings.vue | 2 +- scripts/check-naming-and-style.sh | 20 ++++++++++--- shopdb/core/api/search.py | 18 ++++++----- shopdb/core/api/settings.py | 18 ++++++----- tests/test_core/test_search_integrations.py | 30 ++++++++++++++++++- tests/test_core/test_settings_branding.py | 15 +++++++--- 8 files changed, 91 insertions(+), 42 deletions(-) diff --git a/frontend/src/composables/pageTitle.js b/frontend/src/composables/pageTitle.js index b466eb6f..0b5777b0 100644 --- a/frontend/src/composables/pageTitle.js +++ b/frontend/src/composables/pageTitle.js @@ -1,7 +1,7 @@ // The browser tab title, including WHICH record is open. // // The router can only name the KIND of page, because it runs before the view -// has fetched anything: every application read "West Jefferson ShopDB - +// has fetched anything: every application read " ShopDB - // Application", so two tabs on two different machines were indistinguishable, // and a shared link previewed as the section rather than the thing. // @@ -51,7 +51,7 @@ export async function applyGenericTitle(key, page) { if (key !== activeKey) return // navigated on while we awaited // A view that named the record before the facility name arrived composed its // title against the placeholder prefix, so rewrite it rather than leaving - // 'ShopDB - CSF16' on a site called West Jefferson. + // 'ShopDB - CSF16' without the facility name in front of it. document.title = compose(recordTitle !== null ? recordTitle : page) } diff --git a/frontend/src/utils/siteSettings.js b/frontend/src/utils/siteSettings.js index c7ec67ef..a333a950 100644 --- a/frontend/src/utils/siteSettings.js +++ b/frontend/src/utils/siteSettings.js @@ -98,22 +98,20 @@ function darkenHexColor(hex, fraction) { // ServiceNow ticket-link config. Returns the enabled flag, incident/change URL // templates, and the global-search URL template (all with a {ticket} -// placeholder). Defaults mirror the previously-hardcoded GE ServiceNow URLs. +// placeholder). +// +// The defaults are BLANK and the integration ships OFF, matching the server. +// They used to be one organisation's QA ServiceNow tenant, so any site that had +// not configured ServiceNow still rendered ticket links - pointing its +// technicians at a tenant they cannot reach and, for an adopting site, at +// somebody else's. A blank URL renders tickets as plain text, which is the +// correct behaviour for "not configured". export async function getServicenowUrls() { - const enabledRaw = await getSetting('servicenow_enabled', 'true') + const enabledRaw = await getSetting('servicenow_enabled', 'false') const enabled = enabledRaw !== 'false' && enabledRaw !== '0' && enabledRaw !== false - const incidentUrl = await getSetting( - 'servicenow_incident_url', - 'https://geaerospaceqa.service-now.com/now/nav/ui/search/0f8b85d0c7922010099a308dc7c2606a/params/search-term/{ticket}/global-search-data-config-id/c861cea2c7022010099a308dc7c26041/' - ) - const changeUrl = await getSetting( - 'servicenow_change_url', - 'https://geaerospaceqa.service-now.com/now/nav/ui/search/0f8b85d0c7922010099a308dc7c2606a/params/search-term/{ticket}/global-search-data-config-id/c861cea2c7022010099a308dc7c26041/' - ) - const searchUrl = await getSetting( - 'servicenow_search_url', - 'https://geaerospaceqa.service-now.com/now/nav/ui/search/0f8b85d0c7922010099a308dc7c2606a/params/search-term/{ticket}/global-search-data-config-id/c861cea2c7022010099a308dc7c26041/' - ) + const incidentUrl = await getSetting('servicenow_incident_url', '') + const changeUrl = await getSetting('servicenow_change_url', '') + const searchUrl = await getSetting('servicenow_search_url', '') return { enabled, incidentUrl, changeUrl, searchUrl } } diff --git a/frontend/src/views/settings/ServiceNowSettings.vue b/frontend/src/views/settings/ServiceNowSettings.vue index bfdb8c3f..ffcd7a8b 100644 --- a/frontend/src/views/settings/ServiceNowSettings.vue +++ b/frontend/src/views/settings/ServiceNowSettings.vue @@ -34,7 +34,7 @@ diff --git a/scripts/check-naming-and-style.sh b/scripts/check-naming-and-style.sh index 84e20097..8730db14 100755 --- a/scripts/check-naming-and-style.sh +++ b/scripts/check-naming-and-style.sh @@ -233,17 +233,29 @@ echo "==> Checking for site-specific literals in product code (ADR-015)..." # publication scrub greps for the very strings below - written literally, the # rule's own definition trips the gate that enforces it. Same reason # tests/test_docs_publishable.py splits its terms. -SITE_HOST='tsg'\''wp00525' -SITE_DOMAIN='\.geaero'\''space\.net' -SITE_NAME='West Jeff'\''erson' +# Adjacent quoted strings CONCATENATE: 'tsg''wp00525' is tsgwp00525. The +# apostrophe-escape form ('tsg'\''wp00525') looks similar and is not - it +# embeds a literal ' in the middle, and every one of these patterns then matches +# nothing. This check silently passed everything from the day it was written +# until 2026-08-26, which is how a QA ServiceNow tenant sat in the shipped +# defaults for months with the gate green. +SITE_HOST='tsg''wp00525' +SITE_DOMAIN='\.geaero''space\.net' +SITE_NAME='West Jeff''erson' SITE_NETS='10\.134\.48\.|10\.48\.249\.' -SITE_PATTERNS="${SITE_HOST}|${SITE_DOMAIN}|\bwjs\b|${SITE_NAME}|${SITE_NETS}" +# A named SaaS TENANT is as site-specific as a hostname, and this one shipped as +# the default ServiceNow URL for months: every adopting site would have linked +# its technicians into another organisation's QA instance. The vendor domain +# itself is fine to mention; the tenant is not. +SITE_TENANTS='geaero''spaceqa' +SITE_PATTERNS="${SITE_HOST}|${SITE_DOMAIN}|\bwjs\b|${SITE_NAME}|${SITE_NETS}|${SITE_TENANTS}" SITE_HITS=$(grep -rPni "$SITE_PATTERNS" "${EXCLUDES[@]}" \ --include='*.py' --include='*.vue' --include='*.js' \ --include='*.ps1' --include='*.psm1' --include='*.sh' --include='*.iss' \ --include='*.json' --include='*.html' \ plugins/ shopdb/ scripts/ deploy/ frontend/src/ tools/ 2>/dev/null \ | grep -v '/tests\?/' \ + | grep -v '\.spec\.js:' \ | grep -v 'site_imports/' \ | grep -v 'installer/bundle/' \ | grep -v '\.plugins-staged/' \ diff --git a/shopdb/core/api/search.py b/shopdb/core/api/search.py index 70da6bc1..33fafd10 100644 --- a/shopdb/core/api/search.py +++ b/shopdb/core/api/search.py @@ -72,13 +72,12 @@ def _require_enabled(name): if pm and not pm.registry.is_enabled(name): raise ImportError(f'{name} plugin disabled') -# Shipped GE defaults. Settings override these per-site; identical fallbacks -# live here so this consumer works even if the settings seed has not run. -SERVICENOW_URL_DEFAULT = ( - 'https://geaerospaceqa.service-now.com/now/nav/ui/search/' - '0f8b85d0c7922010099a308dc7c2606a/params/search-term/{ticket}/' - 'global-search-data-config-id/c861cea2c7022010099a308dc7c26041/' -) +# Fallbacks for when the settings seed has not run. The ServiceNow URL is BLANK +# on purpose and must stay that way: a hardcoded tenant here would link one +# site's technicians into another site's ServiceNow, and the code below already +# treats a blank URL as "ServiceNow not configured" and renders tickets as plain +# text. There is no sensible cross-site default for a tenant URL. +SERVICENOW_URL_DEFAULT = '' SERVICENOW_PREFIXES_DEFAULT = 'GEINC,GECHG,GERIT,GESCT' EMPLOYEEID_PATTERN_DEFAULT = r'^\d{9}$' @@ -108,9 +107,12 @@ def _get_search_integrations(): prefixes_raw = SERVICENOW_PREFIXES_DEFAULT prefixes = [p.strip() for p in str(prefixes_raw).split(',') if p.strip()] + # Missing means unconfigured, and unconfigured means OFF - matching the + # shipped default. Defaulting to True here made a site with no ServiceNow + # settings at all behave as though the integration were on. servicenow_enabled = settings.get('servicenow_enabled') if servicenow_enabled is None: - servicenow_enabled = True + servicenow_enabled = False servicenow_url = settings.get('servicenow_search_url') if servicenow_url is None: diff --git a/shopdb/core/api/settings.py b/shopdb/core/api/settings.py index c38d7bdb..66b4ad01 100644 --- a/shopdb/core/api/settings.py +++ b/shopdb/core/api/settings.py @@ -727,20 +727,22 @@ def build_default_settings(): defaults = sitedefaults + brandingdefaults + printingdefaults + identifierdefaults + searchdefaults + mapdefaults + [ # ServiceNow ticket links. Each instance points these at its own # ServiceNow tenant; blank/disabled renders tickets as plain text. + # + # Shipped OFF and BLANK, like the Zabbix block below. These defaults + # used to be one tenant's QA instance, which is wrong twice over: a + # sister site adopting this product would silently link its technicians + # at somebody else's ServiceNow, and even at the originating site a QA + # tenant is not where a real ticket lives. { 'key': 'servicenow_enabled', - 'value': 'true', + 'value': 'false', 'valuetype': 'boolean', 'category': 'integrations', 'description': 'Enable ServiceNow ticket recognition and links in search and dashboards' }, { 'key': 'servicenow_search_url', - 'value': ( - 'https://geaerospaceqa.service-now.com/now/nav/ui/search/' - '0f8b85d0c7922010099a308dc7c2606a/params/search-term/{ticket}/' - 'global-search-data-config-id/c861cea2c7022010099a308dc7c26041/' - ), + 'value': '', 'valuetype': 'string', 'category': 'integrations', 'description': 'ServiceNow global-search URL template. {ticket} is the ticket number.' @@ -754,14 +756,14 @@ def build_default_settings(): }, { 'key': 'servicenow_incident_url', - 'value': 'https://geaerospaceqa.service-now.com/now/nav/ui/search/0f8b85d0c7922010099a308dc7c2606a/params/search-term/{ticket}/global-search-data-config-id/c861cea2c7022010099a308dc7c26041/', + 'value': '', 'valuetype': 'string', 'category': 'integrations', 'description': 'ServiceNow incident URL template. {ticket} is the incident number.' }, { 'key': 'servicenow_change_url', - 'value': 'https://geaerospaceqa.service-now.com/now/nav/ui/search/0f8b85d0c7922010099a308dc7c2606a/params/search-term/{ticket}/global-search-data-config-id/c861cea2c7022010099a308dc7c26041/', + 'value': '', 'valuetype': 'string', 'category': 'integrations', 'description': 'ServiceNow change-request URL template. {ticket} is the change number.' diff --git a/tests/test_core/test_search_integrations.py b/tests/test_core/test_search_integrations.py index 4da87cc2..a6c4d483 100644 --- a/tests/test_core/test_search_integrations.py +++ b/tests/test_core/test_search_integrations.py @@ -14,14 +14,41 @@ from shopdb.core.api.settings import invalidate_settings_cache from shopdb.core.api.search import _get_search_integrations, _classify_query +def _configure_servicenow(): + """Turn ServiceNow on with a tenant URL. + + Required now, and that is the point: the integration ships OFF with a BLANK + URL, because the defaults used to be one organisation's QA tenant and any + adopting site inherited links into somebody else's ServiceNow. + """ + Setting.set('servicenow_enabled', True, valuetype='boolean', + category='integrations') + Setting.set('servicenow_search_url', + 'https://example.service-now.com/search/{ticket}', + valuetype='string', category='integrations') + + def _redirect(client, auth_headers, term): resp = client.get(f'/api/search?q={term}', headers=auth_headers) assert resp.status_code == 200, resp.get_json() return resp.get_json()['data'].get('redirect') +def test_unconfigured_servicenow_yields_no_redirect(client, db, auth_headers): + """Shipped OFF and BLANK. Unconfigured must mean inactive. + + The defaults were once a QA ServiceNow tenant with enabled=true, so a site + that had never configured ServiceNow still sent its technicians to a tenant + they could not reach - and an adopting site to another organisation's. + """ + invalidate_settings_cache() + redirect = _redirect(client, auth_headers, 'GEINC123') + assert redirect is None or redirect.get('type') != 'servicenow' + + def test_default_prefix_classifies_servicenow(client, db, auth_headers): - """With no override, a GE-prefixed ticket redirects to ServiceNow.""" + """Once configured, a default-prefixed ticket redirects to ServiceNow.""" + _configure_servicenow() invalidate_settings_cache() redirect = _redirect(client, auth_headers, 'GEINC123') assert redirect is not None @@ -30,6 +57,7 @@ def test_default_prefix_classifies_servicenow(client, db, auth_headers): def test_custom_prefix_classifies_servicenow(client, db, auth_headers): """A site-configured prefix classifies its tickets as ServiceNow.""" + _configure_servicenow() Setting.set('servicenow_ticket_prefixes', 'ACMEINC', valuetype='string', category='integrations') invalidate_settings_cache() diff --git a/tests/test_core/test_settings_branding.py b/tests/test_core/test_settings_branding.py index 274f1f1f..37c62a56 100644 --- a/tests/test_core/test_settings_branding.py +++ b/tests/test_core/test_settings_branding.py @@ -143,12 +143,19 @@ def test_defaults_contain_new_branding_keys(): def test_defaults_contain_new_integration_keys(): """build_default_settings seeds the ServiceNow integration keys.""" by_key = _defaults_by_key() - assert by_key['servicenow_enabled']['value'] == 'true' + # Ships OFF with blank URLs: the defaults were once a QA tenant, which an + # adopting site would have inherited as links into another organisation. + assert by_key['servicenow_enabled']['value'] == 'false' assert by_key['servicenow_enabled']['category'] == 'integrations' assert by_key['servicenow_ticket_prefixes']['value'] == 'GEINC,GECHG,GERIT,GESCT' - assert '{ticket}' in by_key['servicenow_search_url']['value'] - assert '{ticket}' in by_key['servicenow_incident_url']['value'] - assert '{ticket}' in by_key['servicenow_change_url']['value'] + # Blank, not a tenant URL. There is no cross-site default for "which + # ServiceNow instance is yours", and a blank URL is what the search code + # reads as "not configured" - tickets then render as plain text. + for key in ('servicenow_search_url', 'servicenow_incident_url', + 'servicenow_change_url'): + assert by_key[key]['value'] == '', key + assert '{ticket}' in by_key[key]['description'], \ + 'the placeholder must still be documented for whoever fills it in' def test_defaults_contain_new_site_keys():