From 2565a6db75a5b1726735c04693985698fc0dd535 Mon Sep 17 00:00:00 2001 From: BJ Fulton Date: Thu, 10 Sep 2026 15:34:45 -0600 Subject: [PATCH 1/5] Fix malformed HTTP responses from /capabilities and /availability Both VOSI endpoints emitted their XML document with no HTTP status line and no headers at all -- the first bytes on the wire were `/TAP and returned a 500 whenever that directory did not already exist -- so on a fresh deployment these endpoints failed outright, and on an established one they reached the emitter and produced the header-less response above. Add tests/test_vosi_http_response.py, which asserts on raw socket bytes rather than going through a forgiving client library: a CRLF-terminated status line, no bare LF or CR within any header line, CRLFCRLF ending the header block, an XML Content-type, and an intact body. Verified red/green both ways -- the tests fail against the unpatched code and also catch the `HTTP/1.1 200 OK\n` bare-LF variant. Co-Authored-By: Claude Opus 5 --- TAP/tap.py | 47 +++++++++++++---- tests/test_vosi_http_response.py | 89 ++++++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 10 deletions(-) create mode 100644 tests/test_vosi_http_response.py diff --git a/TAP/tap.py b/TAP/tap.py index 3442d23..9e6cd41 100755 --- a/TAP/tap.py +++ b/TAP/tap.py @@ -570,6 +570,21 @@ def __run__(self, **kwargs): self.statdict['resulturl'] = '' + # + #{ availability and capabilities are static VOSI documents: they need + # no workspace, so answer them before workspace resolution. Going + # through it treated the empty jobid as a workspace name and failed + # with a 500 whenever /TAP did not already exist. + # + if (self.tapcontext == 'availability'): + self.__printVosiAvailability__ () + + if (self.tapcontext == 'capabilities'): + self.__printVosiCapability__ () + # + #} end VOSI static endpoints + # + # # sync or async without input workspace id: make workspace, # otherwise retrieve workspace from getstatus id @@ -660,16 +675,6 @@ def __run__(self, **kwargs): logging.debug(f'workspace = {self.workspace:s}') logging.debug(f'userWorkdir = {self.userWorkdir:s}') - # - #{ if tapcontext is one of vosiEnpoint, take care of take care of VOSI - # output and return - # - if (self.tapcontext == 'availability'): - self.__printVosiAvailability__ () - - if (self.tapcontext == 'capabilities'): - self.__printVosiCapability__ () - # # vositable: make up vositbl filepath # @@ -3037,6 +3042,17 @@ def __printVosiAvailability__ (self, **kwargs): # { printVosiAvailability # + # + # nph- CGI: emit the full HTTP response ourselves. The status + # line and each header must end in CRLF (print supplies the LF), + # and a bare CRLF line closes the header block -- nginx and + # Cloudflare reject the response otherwise. + # + + print ("HTTP/1.1 200 OK\r") + print ("Content-type: text/xml\r") + print ("\r") + print ('') print ('') print ('') print ('') print (' bytes: + """Issue a GET and return the unparsed response bytes off the socket.""" + parts = urlsplit(server) + with socket.create_connection((parts.hostname, parts.port), timeout=30) as sock: + request = ( + f"GET {path} HTTP/1.1\r\n" + f"Host: {parts.hostname}:{parts.port}\r\n" + "Connection: close\r\n" + "\r\n" + ) + sock.sendall(request.encode("ascii")) + chunks = [] + while True: + chunk = sock.recv(65536) + if not chunk: + break + chunks.append(chunk) + return b"".join(chunks) + + +VOSI_PATHS = ["/TAP/capabilities", "/TAP/availability"] + + +@pytest.mark.parametrize("path", VOSI_PATHS) +def test_vosi_status_line_is_crlf_terminated(tap_server: str, path: str): + """The response opens with a CRLF-terminated 200 status line.""" + raw = _raw_get(tap_server, path) + assert raw.startswith(b"HTTP/1.1 200 OK\r\n"), ( + f"{path} must begin with a CRLF-terminated status line; " + f"got {raw[:60]!r}" + ) + + +@pytest.mark.parametrize("path", VOSI_PATHS) +def test_vosi_headers_are_crlf_terminated(tap_server: str, path: str): + """Every header line uses CRLF, and CRLFCRLF separates headers from body.""" + raw = _raw_get(tap_server, path) + assert b"\r\n\r\n" in raw, f"{path} has no CRLF blank line ending the headers" + + head = raw.split(b"\r\n\r\n", 1)[0] + for line in head.split(b"\r\n"): + assert b"\n" not in line, ( + f"{path} header line {line!r} contains a bare LF" + ) + assert b"\r" not in line, ( + f"{path} header line {line!r} contains a bare CR" + ) + + +@pytest.mark.parametrize("path", VOSI_PATHS) +def test_vosi_declares_xml_content_type(tap_server: str, path: str): + """A Content-type header is present and announces XML.""" + raw = _raw_get(tap_server, path) + head = raw.split(b"\r\n\r\n", 1)[0].lower() + assert b"content-type: text/xml" in head, ( + f"{path} is missing an XML Content-type header; got {head!r}" + ) + + +@pytest.mark.parametrize( + "path,root_element", + [ + ("/TAP/capabilities", b"') + assert root_element in body From 4a12ff92cd8f3026ca25defdc6dadf82b082c540 Mon Sep 17 00:00:00 2001 From: BJ Fulton Date: Thu, 10 Sep 2026 16:05:19 -0600 Subject: [PATCH 2/5] Unblock CI: strip trailing whitespace and pin ruff `ruff check .` has been failing on develop since PR #23 merged (2026-06-02), on a W291 trailing-whitespace hit at TAP/vositables.py:214 introduced by 9024259. The lint step runs before pytest, so the whole test matrix aborts before a single test executes. requirements-test.txt asked for `ruff>=0.1`, so CI silently tracked whatever ruff had shipped most recently; a newer ruff began flagging this line on code nobody had touched. Pin ruff==0.15.2 so rule changes arrive as a deliberate bump rather than a surprise red build. The vositables.py change is whitespace-only -- `git diff -w` is empty. Co-Authored-By: Claude Opus 5 --- TAP/vositables.py | 2 +- requirements-test.txt | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/TAP/vositables.py b/TAP/vositables.py index d414081..2b15001 100644 --- a/TAP/vositables.py +++ b/TAP/vositables.py @@ -211,7 +211,7 @@ def __init__(self, **kwargs): # # { Connect to DBMS # - if('connectInfo' in kwargs): + if('connectInfo' in kwargs): self.connectInfo = kwargs['connectInfo'] else: self.msg = 'Required connectInfo dict is missing.' diff --git a/requirements-test.txt b/requirements-test.txt index 343732f..b4f60d6 100644 --- a/requirements-test.txt +++ b/requirements-test.txt @@ -1,5 +1,8 @@ pytest>=7 -ruff>=0.1 +# Pinned: ruff's default rule behaviour changes between releases, and an +# unpinned `ruff>=0.1` turned CI red on untouched code (W291 in +# TAP/vositables.py) the moment a new ruff shipped. Bump deliberately. +ruff==0.15.2 requests # Runtime deps. These are not declared in setup.py's install_requires (a From ca7a755e187d71fa36a844f369c6714271556fc4 Mon Sep 17 00:00:00 2001 From: BJ Fulton Date: Thu, 10 Sep 2026 16:33:00 -0600 Subject: [PATCH 3/5] Cover the fresh-deployment case deterministically The existing VOSI tests go through the session-scoped `tap_server` fixture, which shares one TAP_WORKDIR across the whole suite. By the time they run, earlier sync requests have already created /TAP, so a full-suite run only ever exercised the established-server path -- the fresh-deployment case, where the workspace-resolution bug actually returned a 500, was covered only by accident when the file happened to be run in isolation. Add test_vosi_works_without_existing_workspace, which drives the CGI directly against a pristine per-test workdir and asserts both that the response opens with a CRLF-terminated 200 and that the endpoints create no workspace at all. Verified it pins the second defect specifically: reverting only the dispatch move (keeping the header fix) fails these two tests. Co-Authored-By: Claude Opus 5 --- tests/test_vosi_http_response.py | 63 ++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/tests/test_vosi_http_response.py b/tests/test_vosi_http_response.py index 45397e9..de6a5a3 100644 --- a/tests/test_vosi_http_response.py +++ b/tests/test_vosi_http_response.py @@ -9,11 +9,19 @@ """ from __future__ import annotations +import os import socket +import subprocess +import sys +from pathlib import Path from urllib.parse import urlsplit import pytest +REPO_ROOT = Path(__file__).resolve().parent.parent +FIXTURES = Path(__file__).parent / "fixtures" +STUBS = FIXTURES / "stubs" + def _raw_get(server: str, path: str) -> bytes: """Issue a GET and return the unparsed response bytes off the socket.""" @@ -87,3 +95,58 @@ def test_vosi_body_still_intact(tap_server: str, path: str, root_element: bytes) body = raw.split(b"\r\n\r\n", 1)[1] assert body.lstrip().startswith(b'') assert root_element in body + + +def _run_cgi_with_workdir(tmp_path, fixture_root, path_info: str) -> bytes: + """Run the CGI directly against a *pristine* TAP_WORKDIR. + + The session `tap_server` fixture shares one workdir across the whole + suite, so by the time these tests run `/TAP` has already been + created by earlier sync requests. That hides the fresh-deployment case, + which is exactly where the workspace-resolution bug bit. Point the CGI at + an empty workdir so the no-workspace path is exercised deterministically. + """ + workdir = tmp_path / "workdir" + workdir.mkdir() + conf = tmp_path / "TAP.conf" + conf.write_text( + (FIXTURES / "TAP.conf.template").read_text().format( + TEST_WORKDIR=str(workdir), + TEST_HTTP_URL="http://127.0.0.1:8099", + TEST_DB_PATH=str(fixture_root / "test_data.db"), + TEST_TAP_SCHEMA=str(fixture_root / "tap_schema.db"), + ) + ) + + env = os.environ.copy() + env.update({ + "TAP_CONF": str(conf), + "PATH_INFO": path_info, + "REQUEST_METHOD": "GET", + "QUERY_STRING": "", + "PYTHONPATH": os.pathsep.join([str(REPO_ROOT), str(STUBS)]), + }) + proc = subprocess.run( + [sys.executable, str(fixture_root / "cgi-bin" / "TAP" / "nph-tap.py")], + env=env, capture_output=True, timeout=60, + ) + assert (workdir / "TAP").exists() is False, ( + "the VOSI endpoints must not create a workspace" + ) + return proc.stdout + + +@pytest.mark.parametrize("path", VOSI_PATHS) +def test_vosi_works_without_existing_workspace(tmp_path, fixture_root, path): + """A fresh deployment, where /TAP does not yet exist, still works. + + These endpoints previously fell through to the "retrieve workspace from + jobid" branch with an empty jobid, resolving to /TAP and + returning a 500 whenever that directory was absent. + """ + raw = _run_cgi_with_workdir(tmp_path, fixture_root, path.replace("/TAP", "", 1)) + assert raw.startswith(b"HTTP/1.1 200 OK\r\n"), ( + f"{path} on a pristine workdir must return a CRLF-terminated 200; " + f"got {raw[:120]!r}" + ) + assert b"500" not in raw.split(b"\r\n", 1)[0] From a0955e34a741535ac1a7a25d14e60ff332044922 Mon Sep 17 00:00:00 2001 From: BJ Fulton Date: Thu, 10 Sep 2026 17:06:12 -0600 Subject: [PATCH 4/5] Spell out the CRLF terminator with end='\r\n' Review preference from @jpl-jengelke on #26, mirrored here to keep the two branches in sync. Make the CRLF requirement explicit at each print rather than relying on a trailing \r plus print's implicit \n. Byte output is unchanged -- both forms emit `...\r\n`, and the wire-level tests in tests/test_vosi_http_response.py still pass unmodified -- but the intent is legible without having to remember what print appends, which matters on a file where a bare LF is what broke the endpoint. Co-Authored-By: Claude Opus 5 --- TAP/tap.py | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/TAP/tap.py b/TAP/tap.py index 9e6cd41..05791de 100755 --- a/TAP/tap.py +++ b/TAP/tap.py @@ -3044,14 +3044,16 @@ def __printVosiAvailability__ (self, **kwargs): # # nph- CGI: emit the full HTTP response ourselves. The status - # line and each header must end in CRLF (print supplies the LF), - # and a bare CRLF line closes the header block -- nginx and - # Cloudflare reject the response otherwise. + # line and each header must end in CRLF, and a bare CRLF line + # closes the header block -- nginx and Cloudflare reject the + # response otherwise. The terminator is spelled out via end= + # rather than a trailing \r leaning on print's implicit \n, + # so the CRLF requirement is visible at a glance. # - print ("HTTP/1.1 200 OK\r") - print ("Content-type: text/xml\r") - print ("\r") + print ('HTTP/1.1 200 OK', end='\r\n') + print ('Content-type: text/xml', end='\r\n') + print ('', end='\r\n') print ('') print ('') @@ -3078,14 +3080,16 @@ def __printVosiCapability__ (self, **kwargs): # # nph- CGI: emit the full HTTP response ourselves. The status - # line and each header must end in CRLF (print supplies the LF), - # and a bare CRLF line closes the header block -- nginx and - # Cloudflare reject the response otherwise. + # line and each header must end in CRLF, and a bare CRLF line + # closes the header block -- nginx and Cloudflare reject the + # response otherwise. The terminator is spelled out via end= + # rather than a trailing \r leaning on print's implicit \n, + # so the CRLF requirement is visible at a glance. # - print ("HTTP/1.1 200 OK\r") - print ("Content-type: text/xml\r") - print ("\r") + print ('HTTP/1.1 200 OK', end='\r\n') + print ('Content-type: text/xml', end='\r\n') + print ('', end='\r\n') print ('') print ('') From 202f4d127d42b95307ade28ff435744323d0ade5 Mon Sep 17 00:00:00 2001 From: BJ Fulton Date: Fri, 11 Sep 2026 13:23:08 -0600 Subject: [PATCH 5/5] Serve the VOSI documents as application/xml The NEA deployment already answers /capabilities and /availability with application/xml, and that is the type we want for these documents. Match it here so the two do not diverge, and update the wire-level assertion to match. Scope is deliberately the two VOSI emitters this PR already touches; the other Content-type sites (including __printVosiTables__ and the error paths) are left alone. Co-Authored-By: Claude Opus 5 --- TAP/tap.py | 4 ++-- tests/test_vosi_http_response.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/TAP/tap.py b/TAP/tap.py index 05791de..89d6e2f 100755 --- a/TAP/tap.py +++ b/TAP/tap.py @@ -3052,7 +3052,7 @@ def __printVosiAvailability__ (self, **kwargs): # print ('HTTP/1.1 200 OK', end='\r\n') - print ('Content-type: text/xml', end='\r\n') + print ('Content-type: application/xml', end='\r\n') print ('', end='\r\n') print ('') @@ -3088,7 +3088,7 @@ def __printVosiCapability__ (self, **kwargs): # print ('HTTP/1.1 200 OK', end='\r\n') - print ('Content-type: text/xml', end='\r\n') + print ('Content-type: application/xml', end='\r\n') print ('', end='\r\n') print ('') diff --git a/tests/test_vosi_http_response.py b/tests/test_vosi_http_response.py index de6a5a3..860fb75 100644 --- a/tests/test_vosi_http_response.py +++ b/tests/test_vosi_http_response.py @@ -77,7 +77,7 @@ def test_vosi_declares_xml_content_type(tap_server: str, path: str): """A Content-type header is present and announces XML.""" raw = _raw_get(tap_server, path) head = raw.split(b"\r\n\r\n", 1)[0].lower() - assert b"content-type: text/xml" in head, ( + assert b"content-type: application/xml" in head, ( f"{path} is missing an XML Content-type header; got {head!r}" )