Copilot commented on code in PR #16286:
URL: https://github.com/apache/grails-core/pull/16286#discussion_r3903661861
##########
grails-forge/infrastructure/shared.yaml:
##########
@@ -229,14 +225,20 @@ Resources:
GitHubDeployRole:
Type: AWS::IAM::Role
Properties:
+ ManagedPolicyArns:
+ - Fn::Sub:
arn:${AWS::Partition}:iam::aws:policy/AdministratorAccess-AWSElasticBeanstalk
+ - Fn::Sub:
arn:${AWS::Partition}:iam::aws:policy/AWSElasticBeanstalkWebTier
+ - Fn::Sub:
arn:${AWS::Partition}:iam::aws:policy/AWSElasticBeanstalkManagedUpdatesCustomerRolePolicy
Review Comment:
`GitHubDeployRole` now attaches AWS managed policies including
`AdministratorAccess-AWSElasticBeanstalk`, which substantially broadens the
deployment role’s permissions beyond the previously-scoped inline policy. This
increases blast radius if the role is ever assumed unexpectedly; consider
reverting to least-privilege (or documenting why this breadth is required) and
limiting permissions to the specific Elastic Beanstalk application/environments
used by Forge.
##########
grails-forge/infrastructure/shared.yaml:
##########
@@ -229,14 +225,20 @@ Resources:
GitHubDeployRole:
Type: AWS::IAM::Role
Properties:
+ ManagedPolicyArns:
+ - Fn::Sub:
arn:${AWS::Partition}:iam::aws:policy/AdministratorAccess-AWSElasticBeanstalk
+ - Fn::Sub:
arn:${AWS::Partition}:iam::aws:policy/AWSElasticBeanstalkWebTier
+ - Fn::Sub:
arn:${AWS::Partition}:iam::aws:policy/AWSElasticBeanstalkManagedUpdatesCustomerRolePolicy
AssumeRolePolicyDocument:
Statement:
- Action: sts:AssumeRoleWithWebIdentity
Condition:
StringEquals:
token.actions.githubusercontent.com:aud: sts.amazonaws.com
+ StringLike:
token.actions.githubusercontent.com:sub:
- Fn::Sub:
repo:${GitHubRepository}:ref:refs/heads/${GitHubBranch}
+ - Fn::Sub: repo:${GitHubRepository}:ref:refs/heads/*.x
+ - Fn::Sub: repo:${GitHubRepository}:ref:refs/tags/v*
Review Comment:
The trust policy was broadened from a single configured branch to any
`refs/heads/*.x` branch and any `refs/tags/v*` tag. This meaningfully expands
which refs can assume the deployment role; if you only intend specific
maintenance branches/tags, consider using an explicit allow-list (or
reintroducing a parameter) to avoid granting deploy rights to unrelated `*.x`
branches.
##########
grails-forge/infrastructure/README.md:
##########
@@ -93,4 +93,4 @@ aws cloudformation deploy \
HostedZoneId=<route53-hosted-zone-id>
```
-GitHub Actions assumes the shared stack's `DeployRoleArn`. Upload a normal JAR
deployment ZIP to the exported artifact bucket, create an Elastic Beanstalk
application version, then update one exported environment name. The trust
policy is restricted to the configured repository and branch, and the deploy
policy is restricted to the application, its versions, and the five declared
slot environment names.
+GitHub Actions assumes the shared stack's `DeployRoleArn`. Upload a normal JAR
deployment ZIP to the exported artifact bucket, create an Elastic Beanstalk
application version, then update one exported environment name. The trust
policy allows `apache/grails-core` maintenance branches matching
`refs/heads/*.x` and tags matching `refs/tags/v*`. The deploy policy is
restricted to the application, its versions, and the five declared slot
environment names.
Review Comment:
This README sentence says the deploy policy is restricted, but `shared.yaml`
now also attaches broad AWS managed policies to `GitHubDeployRole` (e.g.
`AdministratorAccess-AWSElasticBeanstalk`). Update the wording so it accurately
reflects the effective permissions granted by the role.
##########
grails-data-mongodb/bson/src/test/groovy/org/grails/datastore/bson/json/JsonReaderSpec.groovy:
##########
@@ -0,0 +1,62 @@
+/*
+ * 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
+ *
+ * https://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.
+ */
+package org.grails.datastore.bson.json
+
+import org.bson.BsonType
+import spock.lang.Specification
+
+/**
+ * Regression coverage for two JsonScanner number-parsing bugs:
+ *
+ * <p>SAW_EXPONENT_DIGITS was the only numeric scanner state whose terminator
switch omitted an
+ * explicit end-of-input case (every sibling state treats EOF the same as a
closing delimiter), so
+ * a bare exponent-notation number with nothing following it (e.g. a top-level
"1e2") threw
+ * JsonParseException instead of parsing.
+ *
+ * <p>SAW_MINUS_I appended the character it read on every iteration of its
"-Infinity" match loop,
+ * including the terminator character read immediately after matching the
literal's final 'y' - so
+ * the buffer handed to Double.parseDouble always carried one extra trailing
character, and
+ * "-Infinity" failed to parse in every context (end-of-input, before a comma,
before a closing
+ * bracket).
+ */
+class JsonReaderSpec extends Specification {
+
+ void "reads a bare exponent-notation number with nothing following it"() {
+ given:
+ JsonReader reader = new JsonReader('1e2')
+
+ expect:
+ reader.readBsonType() == BsonType.DOUBLE
+ reader.readDouble() == 100.0d
+ }
+
+ void "reads -Infinity as negative infinity, at end-of-input and followed
by a delimiter"() {
+ expect:
+ new JsonReader('-Infinity').readBsonType() == BsonType.DOUBLE
+
Review Comment:
The new regression test for top-level `-Infinity` asserts the token type but
not the parsed numeric value. Adding a `readDouble()` assertion here ensures
the fix is exercised end-to-end for the end-of-input case (not just “doesn’t
throw”).
##########
grails-forge/infrastructure/shared.yaml:
##########
@@ -229,14 +225,20 @@ Resources:
GitHubDeployRole:
Type: AWS::IAM::Role
Properties:
+ ManagedPolicyArns:
+ - Fn::Sub:
arn:${AWS::Partition}:iam::aws:policy/AdministratorAccess-AWSElasticBeanstalk
+ - Fn::Sub:
arn:${AWS::Partition}:iam::aws:policy/AWSElasticBeanstalkWebTier
+ - Fn::Sub:
arn:${AWS::Partition}:iam::aws:policy/AWSElasticBeanstalkManagedUpdatesCustomerRolePolicy
Review Comment:
PR title/description focuses on `JsonScanner` number parsing, but this PR
also changes Grails Forge AWS deployment infrastructure (CloudFormation role
trust/permissions and the GitHub Actions deploy workflow). To keep the change
set reviewable and release-notes-friendly, consider splitting the
infrastructure updates into a separate PR or updating the PR description to
explicitly cover these deployment changes and rationale.
--
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]
