Skip to content
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.

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

Fix to not allow parameter entity references at internal subsets #191

Conversation

naitoh
Copy link
Contributor

@naitoh naitoh commented Aug 1, 2024

Why?

In the internal subset of DTD, references to parameter entities are not allowed within markup declarations.

See: https://www.w3.org/TR/xml/#wfc-PEinInternalSubset

Well-formedness constraint: PEs in Internal Subset

In the internal DTD subset, parameter-entity references MUST NOT occur within markup declarations; they may occur where markup declarations can occur. (This does not apply to references that occur in external parameter entities or to the external subset.)

@naitoh naitoh marked this pull request as ready for review August 1, 2024 13:12
@kou kou changed the title Fix to not allow Parameter Entity References at internal subsets. Fix to not allow Parameter Entity References at internal subsets Aug 2, 2024
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

Wow!

Can we also check this in BaseParser?
If we can do it, can we move the validation to #write?

Do we need to update #unnormalized and #normalized too?

# Evaluates to the unnormalized value of this entity; that is, replacing
# all entities -- both %ent; and &ent; entities. This differs from
# +value()+ in that +value+ only replaces %ent; entities.
def unnormalized
document.record_entity_expansion unless document.nil?
v = value()
return nil if v.nil?
@unnormalized = Text::unnormalize(v, parent)
@unnormalized
end

# Returns the value of this entity unprocessed -- raw. This is the
# normalized value; that is, with all %ent; and &ent; entities intact
def normalized
@value
end

lib/rexml/entity.rb Outdated Show resolved Hide resolved
lib/rexml/entity.rb Outdated Show resolved Hide resolved
@naitoh naitoh marked this pull request as draft August 12, 2024 00:55
@naitoh naitoh force-pushed the not_allow_parameter_entity_references_at_internal_subsets branch from ee48121 to 5d75caf Compare August 12, 2024 06:15
@naitoh
Copy link
Contributor Author

naitoh commented Aug 12, 2024

Wow!

Can we also check this in BaseParser?

I changed BaseParser to do this check.

If we can do it, can we move the validation to #write?

I added validation to Entity.new.

Do we need to update #unnormalized and #normalized too?

Updated #unnormalized.
No need to update #normalized.

@naitoh naitoh marked this pull request as ready for review August 12, 2024 06:29
@naitoh naitoh requested a review from kou August 12, 2024 06:30
test/test_entity.rb Outdated Show resolved Hide resolved
@@ -59,6 +60,10 @@ def initialize stream, value=nil, parent=nil, reference=false
@name = stream
@value = value
end

if PEREFERENCE_RE.match?(@value)
raise "Parameter Entity References forbidden in internal subset: #{@value}"
Copy link
Member

Choose a reason for hiding this comment

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

  • Could you use ArgumentError instead of RuntimeError?
  • Could you avoid needless capital case?
Suggested change
raise "Parameter Entity References forbidden in internal subset: #{@value}"
raise ArgumentError, "Parameter entity references forbidden in internal subset: #{@value}"

BTW, do we need a validation here? This method document says the following:

+WARNING+: There is no validation of entity state except when the entity
is read from a stream. If you start poking around with the accessors,
you can easily create a non-conformant Entity.

Copy link
Contributor Author

@naitoh naitoh Aug 17, 2024

Choose a reason for hiding this comment

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

@kou

BTW, do we need a validation here?

OK.
I see.
We prefer not to add this validation in Entity.new.

Similarly, I don't think this validation should be added to #write.
What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with you.

lib/rexml/parsers/baseparser.rb Outdated Show resolved Hide resolved
lib/rexml/parsers/baseparser.rb Outdated Show resolved Hide resolved
test/test_entity.rb Outdated Show resolved Hide resolved
test/test_entity.rb Outdated Show resolved Hide resolved
## Why?
In the internal subset of DTD, references to parameter entities are not allowed within markup declarations.

See: https://www.w3.org/TR/xml/#wfc-PEinInternalSubset

> Well-formedness constraint: PEs in Internal Subset

> In the internal DTD subset, parameter-entity references MUST NOT occur within markup declarations; they may occur where markup declarations can occur. (This does not apply to references that occur in external parameter entities or to the external subset.)
@naitoh naitoh force-pushed the not_allow_parameter_entity_references_at_internal_subsets branch from 5d75caf to eb73ef5 Compare August 17, 2024 00:46
@naitoh naitoh requested a review from kou August 17, 2024 01:03
@kou kou changed the title Fix to not allow Parameter Entity References at internal subsets Fix to not allow parameter entity references at internal subsets Aug 17, 2024
@kou kou merged commit c8110b4 into ruby:master Aug 17, 2024
61 checks passed
@kou
Copy link
Member

kou commented Aug 17, 2024

Thanks.

@naitoh naitoh deleted the not_allow_parameter_entity_references_at_internal_subsets branch August 17, 2024 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants