Skip to content

fix: preserve authorizer config for overlapping API routes #9166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Zahoor-ishfaq wants to merge 9 commits into
base: develop
Choose a base branch
from
Open

fix: preserve authorizer config for overlapping API routes #9166

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
bfb7b0e
fix: preserve authorizer config for overlapping API routes
Zahoor-ishfaq Aug 4, 2026
c5a226e
fix: address route deduplication review feedback
Zahoor-ishfaq Aug 4, 2026
df9f9b1
fix: preserve route ownership during CORS normalization
Zahoor-ishfaq Aug 4, 2026
6b5599b
fix: preserve specific routes during SAM route merge
Zahoor-ishfaq Aug 4, 2026
6f849fb
fix: narrow SAM route preservation
Zahoor-ishfaq Aug 4, 2026
4e8413c
fix: require explicit authorizer intent for route preservation
Zahoor-ishfaq Aug 4, 2026
2535804
fix: prefer unprotected route for synthesized OPTIONS
Zahoor-ishfaq Aug 4, 2026
3a8614f
fix: avoid payload inheritance from preserved routes
Zahoor-ishfaq Aug 4, 2026
a227931
fix: reconcile routes across operation names
Zahoor-ishfaq Aug 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 56 additions & 20 deletions samcli/lib/providers/api_collector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,30 +251,66 @@ def dedupe_function_routes(routes: List[Route]) -> List[Route]:
-------
A list of routes without duplicate routes with the same stack_path, function_name and method
"""
grouped_routes: Dict[str, Route] = {}
grouped_routes: Dict[str, List[Route]] = {}

for route in routes:
key = "{}-{}-{}-{}".format(route.stack_path, route.function_name, route.path, route.operation_name or "")
config = grouped_routes.get(key, None)
methods = route.methods
if config:
methods += config.methods
sorted_methods = sorted(methods)
# Prefer route-specific CORS over None
cors = route.cors if route.cors is not None else (config.cors if config else None)
grouped_routes[key] = Route(
function_name=route.function_name,
path=route.path,
methods=sorted_methods,
event_type=route.event_type,
payload_format_version=route.payload_format_version,
operation_name=route.operation_name,
stack_path=route.stack_path,
authorizer_name=route.authorizer_name,
authorizer_object=route.authorizer_object,
cors=cors,
grouped_routes.setdefault(key, []).append(route)

result: List[Route] = []

def has_same_authorizer(first: Route, second: Route) -> bool:
return (
first.authorizer_name == second.authorizer_name and first.authorizer_object == second.authorizer_object
)
return list(grouped_routes.values())

for route_group in grouped_routes.values():

@aws-sam-tooling-bot aws-sam-tooling-bot Bot Aug 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[GENERAL] The split logic here is correct, but for SAM templates it is only reachable when the explicit OPTIONS event is declared after the ANY event, so the order-dependence the PR aims to remove still exists one layer up.

SamApiProvider.merge_routes() runs before get_api() (see sam_api_provider.py:100) and de-dupes by path + method, keeping the last writer per key:

for config in all_configs:
   # Normalize the methods before de-duping to allow an ANY method in implicit API to override a regular HTTP
   # method on explicit route.
   for normalized_method in config.methods:
       key = config.path + normalized_method
       ...
       all_routes[key] = config

result = set(all_routes.values())  # Assign to a set() to de-dupe

Because Route.normalize_method already expands ANY into all seven verbs, the ANY route claims the /{proxy+}OPTIONS key too. The explicit OPTIONS route only has that one key, so if it is written first and then overwritten, it is no longer a value in all_routes and is dropped by set(all_routes.values()) — dedupe_function_routes never sees it, and the OPTIONS method keeps the ANY route's authorizer.

Concretely, for the template in #9165:

  • ANY event declared first, OPTIONS event second → both routes survive merge_routes → this fix applies.
  • OPTIONS event declared first, ANY second → the OPTIONS route is discarded → preflight is still authorized.
  • ANY event implicit (no RestApiId) and OPTIONS explicit → implicit routes are iterated last by design, so the OPTIONS route is always discarded.

The new tests all call dedupe_function_routes/get_api directly, so they cannot catch this. Please either give the more specific method precedence in merge_routes (an explicit single-method route should not be clobbered by an expanded ANY route) or add a test that drives the scenario through SamApiProvider.extract_resources with the OPTIONS event declared first, so the end-to-end behavior is pinned.

merged_routes: List[Route] = []
group_cors = next((route.cors for route in route_group if route.cors is not None), None)

# Process broader routes first so a more specific route can own
# overlapping methods, e.g. explicit OPTIONS overriding ANY.
for route in sorted(route_group, key=lambda item: len(item.methods), reverse=True):
methods = list(dict.fromkeys(route.methods))

for existing_route in merged_routes:
if not has_same_authorizer(existing_route, route):
existing_route.methods = [method for method in existing_route.methods if method not in methods]

@aws-sam-tooling-bot aws-sam-tooling-bot Bot Aug 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[BUG] Stripping the overlapping methods makes the split disjoint here, but get_api() runs the CORS normalization immediately after dedupe (lines 195-196):

routes = self.dedupe_function_routes(self.routes)
routes = self.normalize_cors_methods(routes, self.cors)

and normalize_cors_methods appends OPTIONS back onto every route that does not already have it:

def add_options_to_route(route: Route) -> Route:
   if "OPTIONS" not in route.methods:
       route.methods.append("OPTIONS")
   return route

So whenever the API has CORS configured, the authorizer-protected route regains OPTIONS and both split routes claim <path>:OPTIONS. LocalApigwService.create() keys _dict_of_routes by path:method only (local_apigw_service.py:145-146), so one entry silently overwrites the other and the winner is whichever route comes later in api.routes. That means the "explicit route owns the overlapping method" rule this PR establishes is re-decided by list position: it currently produces the right result only because the broadest route is appended first by the sort on line 273.

Consider making the released methods explicit instead of relying on ordering — e.g. track the methods a route gave up during dedupe and have normalize_cors_methods skip injecting OPTIONS into a route that deliberately released it (or skip injection when another route in the same group already serves OPTIONS). Otherwise the order-dependence the PR removes from dedupe_function_routes reappears one step later in the pipeline.


matching_route = next(
(existing_route for existing_route in merged_routes if has_same_authorizer(existing_route, route)),
None,
)

if matching_route:
matching_route.methods = sorted(set(matching_route.methods + methods))

@aws-sam-tooling-bot aws-sam-tooling-bot Bot Aug 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[GENERAL] When two routes in a group share an authorizer, the merge only updates methods and cors on the surviving route — which is the first one created, i.e. the route with the most methods (usually the ANY route). Every other attribute of the narrower route is discarded, including payload_format_version.

That one has real consequences for HTTP APIs, because a missing payload format version is treated as 2.0 (local_apigw_service.py:460):

if route.event_type == Route.HTTP and route.payload_format_version in [None, "2.0"]:

So an ANY route with no PayloadFormatVersion now deterministically shadows a sibling method route (same function/path/operation and same authorizer) that declares "1.0", and that method's Lambda receives a v2 event instead of a v1 event. SamApiProvider.merge_routes already guards against exactly this loss:

if route and route.payload_format_version and config.payload_format_version is None:
   config.payload_format_version = route.payload_format_version

Mirroring it in the merge branch keeps the behavior consistent:

if matching_route:
   matching_route.methods = sorted(set(matching_route.methods + methods))
   if matching_route.payload_format_version is None:
       matching_route.payload_format_version = route.payload_format_version
   if route.cors is not None:
       matching_route.cors = route.cors
   continue

The same one-sided loss applies to use_default_authorizer, which the PR description says is preserved: it is preserved on the copy (line 302) but on a merge only the first route's value survives, and test_merges_routes_with_same_resolved_authorizer locks that in (the False from the POST route is dropped). That is currently inert because _link_authorizers() has already run, but it is worth a comment in the code so the next reader does not assume the flag is still meaningful.

if route.cors is not None:
matching_route.cors = route.cors
continue

merged_routes.append(
Route(
function_name=route.function_name,
path=route.path,
methods=sorted(methods),
event_type=route.event_type,
payload_format_version=route.payload_format_version,
operation_name=route.operation_name,
stack_path=route.stack_path,
authorizer_name=route.authorizer_name,
authorizer_object=route.authorizer_object,
use_default_authorizer=route.use_default_authorizer,
cors=route.cors,

@aws-sam-tooling-bot aws-sam-tooling-bot Bot Aug 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[BUG] Route-level CORS is no longer propagated across the routes in a group. The removed code deliberately merged it:

# Prefer route-specific CORS over None
cors = route.cors if route.cors is not None else (config.cors if config else None)

The new code only carries cors forward inside the if matching_route: branch, i.e. only between routes that share an authorizer config. When a group now splits because the authorizers differ, the non-OPTIONS route is constructed with cors=route.cors, which is None for every non-OPTIONS method — cfn_api_provider.py only ever attaches route-level CORS to OPTIONS methods:

cors=cors if method == "OPTIONS" else None,

There is no fallback to recover it. In local_apigw_service._request_handler:

cors = route.cors if route.cors is not None else self.api.cors
...
headers.update(cors_headers)

self.api.cors is not set in this path either, because cfn_api_provider assigns collector.cors only in the elif cors: branch for non-OPTIONS methods. So for a REST API whose OPTIONS method carries the CORS integration responses while its other methods carry an authorizer, the preflight still succeeds but the actual GET/POST response is returned with no Access-Control-Allow-* headers, and the browser rejects it. Previously the single merged route held cors and all methods got the headers.

Compute the group's effective CORS once and apply it to every resulting route that has none of its own:

group_cors = next((route.cors for route in route_group if route.cors is not None), None)
...
for merged_route in merged_routes:
   if merged_route.cors is None:
       merged_route.cors = group_cors

result.extend(route for route in merged_routes if route.methods)

)
)

for merged_route in merged_routes:
if merged_route.cors is None:
merged_route.cors = group_cors

result.extend(route for route in merged_routes if route.methods)

return result

def add_binary_media_types(self, logical_id: str, binary_media_types: Optional[List[str]]) -> None:
"""
Expand Down
87 changes: 87 additions & 0 deletions tests/unit/commands/local/lib/test_api_collector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,3 +160,90 @@ def test_link_authorizers(self, routes, authorizers, default_authorizer, expecte
self.api_collector._link_authorizers()

self.assertEqual(self.api_collector._route_per_resource, {self.apigw_id: expected_routes})


class TestApiCollector_dedupe_function_routes(TestCase):
def test_preserves_options_route_with_different_authorizer(self):
routes = [
Route(
function_name="func",
path="/{proxy+}",
methods=["ANY"],
authorizer_name="MyAuthorizer",
),
Route(
function_name="func",
path="/{proxy+}",
methods=["OPTIONS"],
authorizer_name=None,
use_default_authorizer=False,
),
]

actual = ApiCollector.dedupe_function_routes(routes)

expected = [
Route(
function_name="func",
path="/{proxy+}",
methods=["GET", "DELETE", "PUT", "POST", "HEAD", "PATCH"],
authorizer_name="MyAuthorizer",
),
Route(
function_name="func",
path="/{proxy+}",
methods=["OPTIONS"],
authorizer_name=None,
use_default_authorizer=False,
),
]

self.assertCountEqual(expected, actual)

def test_preserves_cors_when_routes_split_by_authorizer(self):
cors = object()

routes = [
Route(
function_name="func",
path="/{proxy+}",
methods=["ANY"],
authorizer_name="MyAuthorizer",
),
Route(
function_name="func",
path="/{proxy+}",
methods=["OPTIONS"],
authorizer_name=None,
use_default_authorizer=False,
cors=cors,
),
]

actual = ApiCollector.dedupe_function_routes(routes)

self.assertEqual(len(actual), 2)
self.assertTrue(all(route.cors is cors for route in actual))

def test_merges_routes_with_same_resolved_authorizer(self):
routes = [
Route(
function_name="func",
path="/x",
methods=["GET"],
authorizer_name=None,
use_default_authorizer=True,
),
Route(
function_name="func",
path="/x",
methods=["POST"],
authorizer_name=None,
use_default_authorizer=False,
),
]

actual = ApiCollector.dedupe_function_routes(routes)

self.assertEqual(len(actual), 1)
self.assertEqual(sorted(actual[0].methods), ["GET", "POST"])