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

Catch ofxget scan exceptions, allow V1 headers to support later versions #181

Merged
merged 1 commit into from
Jan 13, 2024

Conversation

plummer86
Copy link
Contributor

One of my banks returns OFXHeader V100 and VERSION 203 content.
When ofxget scans with V200 headers, the server returns html error message content which fails OFX header parsing as expected. However the OFXHeaderError exceptions were not caught by ofxget - read_scan_response and the command terminates.
This was preventing me from scanning and getting acct info. Updated ofxget to catch the OFXHeaderErrors.

Secondly, OFXHeaderV1 was updated to relax verification of version. My understanding of the OFX standard is that V1XX can support V2XX messages.

@csingley
Copy link
Owner

Updated ofxget to catch the OFXHeaderErrors.

oh good.

My understanding of the OFX standard is that V1XX can support V2XX messages.

It can?? Got a reference?

@plummer86
Copy link
Contributor Author

My understanding of the OFX standard is that V1XX can support V2XX messages.

It can?? Got a reference?

Sure, in the v2.3 standard here, section 1.4 OFX Versions:

Version 1.5.1 supports all version 2 message sets, Bill Presentment, and all version 1 message sets.
Because it supports all message sets, the OFX 1.5.1 DTD can be used to create and support OFX 1.0.2 and/
or OFX 1.5.1 clients and servers.

My interpretation is that OFX v1.5.1+ can use V100 OFXHEADER/SGML with V2 messages.

From https://cdn.nsoftware.com/help/BOF/acx/CCStatement_p_OFXVersion.htm:

Note: If the OFXVersion is set to 1x, the request and the response are going to be in SGML format. If set to 2x, the request will be in XML format (the response format depends on the FI's server capabilities). Note that if the OFX FI server does not support version 2x, the server will return an error (such as 'Bad Request').

@csingley
Copy link
Owner

csingley commented Jan 2, 2024

Pulled the spec to check. Sadly OFX docs are now hosted by FDX, the level of care of whose stewardship is indicated by their bungling the link for OFXv1.0.3 ... which, despite their earnestly expressed wishes, still seems to have a dominant market position with FIs AFAICT.

The VERSION field in the header refers to the controlling DTD... so these metadata naturally can't be covered by the DTD, even if we could get around their non-SGML format. The controlling specification is therefore the word doc, specifically section 2.2.3. Each version of the spec enumerates the valid DTDs acceptable to that version. The latest spec (v1.6) says:

The current accepted values for VERSION are 102, 151, and 160.

Trying hard to hold the pillow down tight on the face of v103.... but in any case, no version of the spec prior to OFXv2 accepts a DTD version higher than its own (naturally enough).

The stuff about "message sets" means something a little different than maybe you might think - they're talking about the data structures that define the protocol for features like banking, investments, credit cards, billpay, etc. ofxtools doesn't make any attempt to validate msgset availability & restrict to claimed version... I didn't quite make it to 100% coverage of every version of the spec, but my intention definitely matches what you quoted from the V2 spec:

supports all version 2 message sets, Bill Presentment, and all version 1 message sets.

However, adding a missing message set involves substantially more work than just relaxing a validator in the header parser, q.v. the docs for an example.

Be that as it may - your FI is violating at least one aspect of the OFX spec... the combination of OFXHEADER=100 and version=203 is invalid.

It sounds like they're also violating the protocol behavior specified in section 1.4, just above the bit you quoted:

There are several distinct versions of OFX clients and servers. An OFX server responds "in kind" to the version of the OFX request. So an OFX client wanting to speak OFX 2.2.1 would send 221 in the appropriate header in the request and the server would respond "in kind". This implies:
• The client sends the latest VERSION: header value that it can support (e.g. VERSION:221)
• The server echoes that value, or a lower value if that’s all it can support (e.g. VERSION:220)
• If the client sends an older VERSION than the server supports, the server should echo that VERSION in the response and more importantly, it must return only tags that the client can handle.

If you saw ofxget sending OFXv1 headers setting VERSION: 203, do please file that as a bug.

As for your own FI's particular clown show... in general I am not favorably inclined to step on the endless whack-a-mole treadmill of punching holes in the conformance validation for every single lazy sloppy mutation committed by some FI somewhere; there are hundreds of them & I'm well over running around cleaning up FIs who aren't toilet trained. We're their customers... and not to put too fine a point on it, they're getting paid for this, not me.

But then you did come bearing not only a minimally invasive fix, but also fixing up the tests... and really I'm not sure we gain anything significant by enforcing this validation, since (as mentioned above) we don't really proceed to make any particular use of the DTD version claimed in the metadata. Effectively this validator functions only as a sanity check (working fine, since your data is indeed insane) .... but not an especially important one... and it doesn't look like other users of the library will need to bear any real cost to accommodate your FI's clownshow, which is what really matters.

Apologies, none of this is your fault & I don't intend to be obstructionist. A moment more to consider the issue.

Regards

@plummer86
Copy link
Contributor Author

Apologies, none of this is your fault & I don't intend to be obstructionist. A moment more to consider the issue.

No worries, I understand and agree with your considerations. Let me know if you think of another path I can explore.

@csingley
Copy link
Owner

Objections here some more theoretical than practical. Hard to characterize real downside to set against demonstrated upside.

Ship it baby.

@csingley csingley merged commit cd280e7 into csingley:master Jan 13, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants