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 3 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
112 changes: 85 additions & 27 deletions samcli/lib/providers/api_collector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,8 @@ def get_api(self) -> Api:
@staticmethod
def normalize_cors_methods(routes: List[Route], cors: Optional[Cors]) -> List[Route]:
"""
Adds OPTIONS method to all the route methods if cors exists
Adds OPTIONS method to route methods if cors exists while preserving
explicit OPTIONS ownership within a route group.

Parameters
-----------
Expand All @@ -229,15 +230,29 @@ def normalize_cors_methods(routes: List[Route], cors: Optional[Cors]) -> List[Ro

Return
-------
A list of routes without duplicate routes with the same function_name and method
A list of routes with at most one OPTIONS owner per route group
"""
if not cors:
return routes

def add_options_to_route(route: Route) -> Route:
if "OPTIONS" not in route.methods:
route.methods.append("OPTIONS")
return route
grouped_routes: Dict[str, List[Route]] = {}

return routes if not cors else [add_options_to_route(route) for route in routes]
for route in routes:
key = "{}-{}-{}-{}".format(route.stack_path, route.function_name, route.path, route.operation_name or "")
grouped_routes.setdefault(key, []).append(route)

result: List[Route] = []

for route_group in grouped_routes.values():
options_claimed = any("OPTIONS" in route.methods for route in route_group)

@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] options_claimed short-circuits the new unauthenticated-owner preference whenever the group contains an ANY-derived route, because Route.normalize_method expands ANY to ANY_HTTP_METHODS, which always includes OPTIONS. So the docstring's claim that "synthesized OPTIONS prefers a route without a linked local authorizer" does not hold for the most common CORS shape.

Concretely, an AWS::Serverless::Api with Cors set and a group of ANY /x (authorizer MyAuth) plus POST /x (security: []): dedupe_function_routes strips POST from the ANY route but leaves OPTIONS on it, so options_claimed is True and OPTIONS stays owned by the authorizer-protected route. _request_handler dispatches on route.authorizer_object, so the local preflight is challenged. On AWS the Cors property causes the transform to emit an unauthenticated OPTIONS mock integration that overrides ANY, so preflight succeeds there — a browser-visible local/deployed divergence.

The check that matters is not "does any route in the group list OPTIONS" but "does any route explicitly declare OPTIONS". An OPTIONS entry that only exists because ANY was expanded is not an ownership claim, and when cors is configured it should be re-assignable to an unauthorized sibling the same way an explicitly declared OPTIONS route already takes precedence in dedupe_function_routes. As written, the preference only ever fires for groups made up entirely of single-method routes — which is what the new tests cover.


for route in route_group:
if not options_claimed:

@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] The synthesized OPTIONS owner is chosen by group iteration order, so it can land on an authorizer-protected route even when an unauthenticated sibling exists in the same group.

After the dedupe_function_routes change, a group can legitimately contain several routes with different authorizers. Consider an AWS::Serverless::Api with Cors set and a DefinitionBody where /x get has security: [{MyAuth: []}] and /x post has security: []. The Swagger parser gives both the same backing function, no operationId, and the same path, so they share the dedupe group key and now survive as two separate routes ([GET] with MyAuth, [POST] with none). Neither contains OPTIONS, so options_claimed is False and the first route in the group — Swagger paths declaration order — gets OPTIONS appended.

If that first route is get, preflight is served by a route carrying authorizer_object, and _request_handler invokes the Lambda authorizer for it. Browser preflight requests do not carry credentials, so the preflight fails with 401/403 and the real request is never sent. Before this PR these routes were collapsed into one, so this selection did not exist.

Since keeping preflight out of the authorizer path is the point of the PR, pick the owner deliberately rather than by position:

for route_group in grouped_routes.values():
   options_claimed = any("OPTIONS" in route.methods for route in route_group)

   if not options_claimed:
       # Prefer a route without an authorizer so CORS preflight is not authorized
       owner = next(
           (route for route in route_group if route.authorizer_object is None),
           route_group[0],
       )
       owner.methods.append("OPTIONS")

   result.extend(route_group)

route.methods.append("OPTIONS")
options_claimed = True
result.append(route)

return result

@staticmethod
def dedupe_function_routes(routes: List[Route]) -> List[Route]:
Expand All @@ -251,30 +266,73 @@ 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 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

# Authorizers are already resolved by _link_authorizers() before
# deduplication, so use_default_authorizer does not affect this merge.
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
180 changes: 180 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,183 @@ 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"])

def test_cors_normalization_does_not_readd_options_to_protected_route(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,
),
]

deduped_routes = ApiCollector.dedupe_function_routes(routes)
actual = ApiCollector.normalize_cors_methods(deduped_routes, object())

options_routes = [route for route in actual if "OPTIONS" in route.methods]

self.assertEqual(len(options_routes), 1)
self.assertIsNone(options_routes[0].authorizer_name)

def test_preserves_payload_format_version_when_merging_routes(self):
routes = [
Route(
function_name="func",
path="/x",
methods=["ANY"],
event_type=Route.HTTP,
authorizer_name=None,
),
Route(
function_name="func",
path="/x",
methods=["GET"],
event_type=Route.HTTP,
payload_format_version="1.0",
authorizer_name=None,
),
]

actual = ApiCollector.dedupe_function_routes(routes)

self.assertEqual(len(actual), 1)
self.assertEqual(actual[0].payload_format_version, "1.0")

def test_get_api_preserves_explicit_unauthorized_options_with_cors(self):
collector = ApiCollector()

authorizer = Authorizer(
authorizer_name="MyAuthorizer",
type="request",
payload_version="1.0",
)

collector.add_authorizers("api", {"MyAuthorizer": authorizer})
collector.set_default_authorizer("api", "MyAuthorizer")

collector.add_routes(
"api",
[
Route(
function_name="func",
path="/{proxy+}",
methods=["ANY"],
),
Route(
function_name="func",
path="/{proxy+}",
methods=["OPTIONS"],
authorizer_name=None,
use_default_authorizer=False,
),
],
)

collector.cors = object()

api = collector.get_api()

options_routes = [route for route in api.routes if "OPTIONS" in route.methods]
get_routes = [route for route in api.routes if "GET" in route.methods]

self.assertEqual(len(options_routes), 1)
self.assertIsNone(options_routes[0].authorizer_name)

self.assertEqual(len(get_routes), 1)
self.assertEqual(get_routes[0].authorizer_name, "MyAuthorizer")
self.assertIs(get_routes[0].authorizer_object, authorizer)